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

None values in lattice_vectors #170

Closed
CasperWA opened this issue Feb 6, 2020 · 4 comments · Fixed by #171
Closed

None values in lattice_vectors #170

CasperWA opened this issue Feb 6, 2020 · 4 comments · Fixed by #171
Assignees
Labels
priority/high Issue or PR with a consensus of high priority

Comments

@CasperWA
Copy link
Member

CasperWA commented Feb 6, 2020

It seems from the definition of lattice_vectors that while the values may be None, they should be it for a vector as a whole.

This should be checked in the models by a pydantic validator.

@CasperWA CasperWA added the priority/high Issue or PR with a consensus of high priority label Feb 6, 2020
@CasperWA CasperWA self-assigned this Feb 6, 2020
@ml-evs
Copy link
Member

ml-evs commented Feb 7, 2020

Relevant part of the spec:

This property MUST be an array of dimensions 3 times 3 regardless of the elements of dimension_types. The vectors SHOULD by convention be chosen so the determinant of the lattice_vectors matrix is different from zero. The vectors in the non-periodic directions have no significance beyond fulfilling these requirements.
All three elements of the inner lists of floats MAY be null for non-periodic dimensions, i.e., those dimensions for which dimension_types is 0.

Which part indicates that they all have to be null? It seems to me that any non-null values should just be ignored.

@CasperWA
Copy link
Member Author

CasperWA commented Feb 7, 2020

This part:

  • All three elements of the inner lists of floats MAY be null for non-periodic dimensions, i.e., those dimensions for which dimension_types is 0.

But I am still not sure this always falls out to be a complete lattice vector?

Concerning the determinant, we can validate that as well if you want? It would probably make sense, but since it is SHOULD, it also isn't REQUIRED, so it shouldn't raise if not true, but instead maybe return a warning?

@ml-evs
Copy link
Member

ml-evs commented Feb 7, 2020

It's not clear to me that the sentence is stipulating that all values must be null if one is, though it probably should.

I don't have strong feelings about the determinant. We could validate it in the case of dimension_types: [1, 1, 1] but don't think we want to go down the route of validating it in <3D...

@shyamd
Copy link
Contributor

shyamd commented Feb 7, 2020

I agree with @ml-evs that it doesn't imply that it has to be null but rather null is only allowed when that direction is aperiodic. We already have a validator for this:

def required_if_dimension_types_has_one(cls, v, values):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/high Issue or PR with a consensus of high priority
Development

Successfully merging a pull request may close this issue.

3 participants