OPTIMADE icon indicating copy to clipboard operation
OPTIMADE copied to clipboard

Property 'fractional_site_positions'

Open merkys opened this issue 4 years ago • 7 comments

In #196 @rartino and I discussed the need to have a structures property for reduced (a.k.a. fractional) atom coordinates. This PR introduces fractional_site_positions among the standardized properties. I have copied the most of the text from cartesian_site_positions and slightly adjusted the relationships of the two coordinate-based properties. I have also changed a bit the definition of lattice_vectors, thus this PR fixes #196.

merkys avatar Nov 22 '19 11:11 merkys

The way you have implemented this clearly requires the cartisian_site_positions and fractional_site_positions to be in the exact same order - line-by-line referring to the same coordinates. I think that requirement should be written down somewhere; I suggest in the list of requirements on fractional_site_positions.

Requirement added in 8eefdd2.

merkys avatar Nov 25 '19 09:11 merkys

As I've written in other issues/commits, I very much like the idea of standardizing fractional_site_positions. But I realize now with this PR in hand that if we do it this way, fractional_site_positions and cartesian_site_positions become "equal citizens". I.e., it would be OK for an implementation to support only the fractional_site_positions part of the specification and refuse to deal with cartesian_site_positions. This is a problem from a standards-perspective. Can we solve that somehow?

In this PR cartesian_site_positions are defined as REQUIRED, and fractional_site_positions as OPTIONAL. However, if PRs #203 or #210 are accepted, both properties will become OPTIONAL with no requirement on their support.

Now I'm starting to understand the importance of REQUIRED in the response constraint. In the light of PRs #203 and #210 this constraint should be transformed to REQUIRED to be supported, that is, a property has to be required to return meaningful values upon request.

Edit: Actually, I've run through the specification, and it seems that even properties that MUST be included in the response may still be included with nulls for values. Thus we nowhere require any properties to be universally supported. Even a JSON document with all properties having null values is valid according to the specification. Or am I missing something?

merkys avatar Nov 26 '19 07:11 merkys

Edit: Actually, I've run through the specification, and it seems that even properties that MUST be included in the response may still be included with nulls for values. Thus we nowhere require any properties to be universally supported. Even a JSON document with all properties having null values is valid according to the specification. Or am I missing something?

I have been concerned about this since we started allowing null-valued properties, and I think the question was raised on the last physical meeting if we really mean to allow returning null for a REQUIRED property.

However, with null being allowed for REQUIRED properties these two alternatives have a rather different result:

  • For every type of data OPTiMaDe supports, there is only a single format to represent it. Implementations either provide data in that format, or return null.
  • OPTiMaDe allows the same data to be returned in N number of ways, e.g., cartesian_site_positions and fractional_site_positions and implementations can choose to return null for either one of them (or both).

In the first case, we still have interoperability for all cases where it makes sense. A database either does not support coordinates at all (in which case interoperability with a client that wants coordinates is not possible) or it supports coordinates via cartesian_site_coordinates in which case a client that wants coordinates gets them as expected.

In the second case, we set ourselves up for a possible mismatch. The client wants coordinates from cartesian_site_positions, but the database returns null for that, and only supports coordinates via fractional_site_positions. This breaks interoperability.

Hence, it is my firm belief that if we are to accept the present PR, there needs to be some facility that says that IF you support fractional_site_positions, you MUST support cartesian_site_positions, but not the other way around. I.e., which makes cartesian_site_positions "more standard" than fractional_site_positions.

With the realization that we are opening a can of worms by allowing more than one standardized way to represent data, perhaps we need to delay this PR beyond 1.0 and sort out exactly how this should be handled?

rartino avatar Dec 02 '19 00:12 rartino

Did the consensus evolve on this at either of the last 2 workshops? If this is something we still cannot deal with, I would suggest closing this PR (even if only temporarily).

ml-evs avatar Jul 07 '21 11:07 ml-evs

Did the consensus evolve on this at either of the last 2 workshops? If this is something we still cannot deal with, I would suggest closing this PR (even if only temporarily).

I don't think we yet have a resolution to my comment above:


Hence, it is my firm belief that if we are to accept the present PR, there needs to be some facility that says that IF you support fractional_site_positions, you MUST support cartesian_site_positions, but not the other way around. I.e., which makes cartesian_site_positions "more standard" than fractional_site_positions.


But the PR is good otherwise, so isn't it better to mark it with a label that means that there is something fundamental that needs to be worked out, but leaving it open?

rartino avatar Jul 07 '21 13:07 rartino

I prefer keeping the PR open as well. Once closed, a PR becomes virtually invisible and it is easy to forget it, scrapping all the effort.

merkys avatar Jul 08 '21 04:07 merkys

It is my understanding that this is going to be superseded by asym_fractional_coords in a PR spawned by #416. (No one has so far suggested that we should include both a "filled cell" and an "asymetric unit" version of fractional coords).

rartino avatar Dec 30 '22 22:12 rartino