Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should atomic properties be REQUIRED? #285

Open
giovannipizzi opened this issue Jun 12, 2020 · 6 comments
Open

Should atomic properties be REQUIRED? #285

giovannipizzi opened this issue Jun 12, 2020 · 6 comments
Labels
misc/question Further information is requested

Comments

@giovannipizzi
Copy link
Contributor

See e.g. this comment and the following.

In particular:
dimension_types is optional, but they are important. E.g., a Si_2 molecule and a Si crystal might only differ on the dimension_types, and they are very different things.

Or cartesian_site_positions, species_at_sites, species.

Or at least, should we say that if one is present, all MUST be present?

@giovannipizzi giovannipizzi added the misc/question Further information is requested label Jun 12, 2020
@sauliusg
Copy link
Contributor

dimension_types is optional, but they are important. E.g., a Si_2 molecule and a Si crystal might only differ on the dimension_types, and they are very different things.

Or cartesian_site_positions, species_at_sites, species.

Or at least, should we say that if one is present, all MUST be present?

I remember @merkys (pinging him on this) argumented that making cartesian_site_positions required may put undue stress on servers that compute them on-the-fly (our's does) and will delay response and waste bandwidth for clients that do not need this information (e.g. you are only interested in cell volumes and crystal densities). I would appreciate if Andrius comments more on this point.

@giovannipizzi
Copy link
Contributor Author

I see. And now I remember the use case of not giving atomic positions.

However, if atomic positions are given, I think we want to give also the rest of the arrays, right?

@rartino
Copy link
Contributor

rartino commented Jun 13, 2020

I'm a bit reluctant to phrase these requirements in the specification, i.e., if A is not null then B must also not be null. In a previous round we really tried to simplify the requirements on properties in terms of their querability and null-ness so the options fall into a few categories, rather than having to be hand-coded for each individual property.

@merkys
Copy link
Member

merkys commented Jun 15, 2020

I remember @merkys (pinging him on this) argumented that making cartesian_site_positions required may put undue stress on servers that compute them on-the-fly (our's does) and will delay response and waste bandwidth for clients that do not need this information (e.g. you are only interested in cell volumes and crystal densities). I would appreciate if Andrius comments more on this point.

Current COD supports coordinate-based properties by request, thus it adheres to MUST requirement already despite the computations it takes. So I am for making the requirement stricter.

@ml-evs
Copy link
Member

ml-evs commented Nov 4, 2020

We've just run into this again over at Materials-Consortia/optimade-python-tools#560. The question is whether the specification requires values of SHOULD fields depending on the values of other SHOULD fields, e.g. if dimension_types = [1, 1, 0], is nperiodic_dimensions still allowed to be null? From my current reading of the specification, I would lean towards yes, as any ambiguity should be resolved in the least strict way (at least for validation).

Currently the validator does not allow this, as the check for consistency between correlated fields like these also includes a check for nullity. The PR linked above relaxes this constraint and allows all SHOULD fields to be null, no matter the values of other fields.

What is unclear to @CasperWA and myself is whether this is an open question on the interpretation of v1.0 of the specification, or whether the above discussion is a proposal for modifying the specification in, say, v1.1.

In terms of our implementation, we are considering a process whereby a warning is emitted if one of two correlated fields is not provided, with an error only being raised if the values are inconsistent. This is already the case for the "SHOULD" field assemblies being present if the "MUST" field structure_features contains the value 'assemblies'.

@merkys
Copy link
Member

merkys commented Nov 4, 2020

We've just run into this again over at Materials-Consortia/optimade-python-tools#560. The question is whether the specification requires values of SHOULD fields depending on the values of other SHOULD fields, e.g. if dimension_types = [1, 1, 0], is nperiodic_dimensions still allowed to be null? From my current reading of the specification, I would lean towards yes, as any ambiguity should be resolved in the least strict way (at least for validation).

I would think the same. If the correlation of fields is not explicitly established in the specification, then I guess they should be treated as uncorrelated.

What is unclear to @CasperWA and myself is whether this is an open question on the interpretation of v1.0 of the specification, or whether the above discussion is a proposal for modifying the specification in, say, v1.1.

Good question. It would be interesting to look into how other specification projects deal with reducing ambiguity :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
misc/question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants