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

Adding a warning to apply_symop() #134

Closed
DHekstra opened this issue Mar 7, 2022 · 4 comments · Fixed by #171
Closed

Adding a warning to apply_symop() #134

DHekstra opened this issue Mar 7, 2022 · 4 comments · Fixed by #171
Labels
bug Something isn't working enhancement Improvement to existing feature

Comments

@DHekstra
Copy link
Contributor

DHekstra commented Mar 7, 2022

When converting from a centered spacegroup like C2 to a non-centered one like P1, it is natural to apply a symop like
gemmi.Op("1/2*x-1/2*y,1/2*x+1/2*y,z"). When not careful with the input Miller indices, this can result in fractional Miller indices. It may be good to add a check that new indices are integers.

@JBGreisman
Copy link
Member

Regarding changing spacegroups, we have a long-standing issue (#5) to address this -- exactly because of all the gotchas that can come up with regard to centering. It hasn't been a high priority for now and will require some diligence with regard to testing to make sure we get it all right.

I think that sort of check for fractional Miller indices could be a good idea though because it would cause issues in other places in rs.

@JBGreisman JBGreisman added the enhancement Improvement to existing feature label Mar 7, 2022
@kmdalton
Copy link
Member

kmdalton commented Mar 7, 2022

This maybe should even raise an error. I'm fine with whatever you all decide.

@JBGreisman
Copy link
Member

JBGreisman commented Mar 17, 2022

Just to expand on this, the current behavior when this arises is to coerce the result to an integer value. This can result in something that isn't the applied symmetry operation. This occurs because of the implementation using np.floor_divide() in rs.utils.apply_to_hkl():

https://github.com/Hekstra-Lab/reciprocalspaceship/blob/ae0e2f538f0d287a7003ce056a7a2613586d4fbf/reciprocalspaceship/utils/symop.py#L4-L20

I can see two solutions:

  1. Implement in rs.utils.apply_to_hkl() -- we can change this function to use np.divide() and raise a warning or error if the resulting array has non-integer parts. The checking for non-integer parts could be handled with something like not np.any(np.mod(Hnew, 1)), however the error-checking does run the risk of slowing down functions that heavily rely on apply_to_hkl() such as rs.utils.hkl_to_asu() (we test whether this really is a bottleneck though).
  2. Implement error-checking in DataSet.apply_symop() -- this will require modifying rs.utils.apply_to_hkl() to use np.divide, and letting other functions handle the testing of this. My hesitation here is that it could require re-implementing the same checks in different places. However, by construction, hkl_to_asu() won't produce fractional Miller indices, so these checks may only be needed in the more general methods like apply_symop()

@JBGreisman
Copy link
Member

Looking back at this, one can detect gemmi.Op instances that won't run into this problem:

import gemmi

# Can't produce non-integer HKLs if provided integer HKLs
op = gemmi.Op("x,x-y,2*x-z")
print(((np.array(op.rot)/op.DEN)%1==0).all())       # prints True

# Can cause problems given integer HKL input
op = gemmi.Op("1/2*x-1/2*y,1/2*x+1/2*y,z")
print(((np.array(op.rot)/op.DEN)%1==0).all())       # prints False

The test if ((np.array(op.rot)/op.DEN)%1==0).all() can be used to go between the two operating modes for this function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement Improvement to existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants