rs-station / reciprocalspaceship

Tools for exploring reciprocal space
https://rs-station.github.io/reciprocalspaceship/
MIT License
28 stars 12 forks source link

Fix sign of phase shift in DataSet.apply_symop #63

Closed JBGreisman closed 3 years ago

JBGreisman commented 3 years ago

Based on visual inspection of maps, it appeared that DataSet.apply_symop() was applying phase shifts in the opposite direction of the specified symmetry operation. This PR changes the sign for how phase shifts are applied, and updates the relevant test.

JBGreisman commented 3 years ago

To do:

codecov-commenter commented 3 years ago

Codecov Report

Merging #63 (15c41a7) into main (cb5b764) will decrease coverage by 0.05%. The diff coverage is 95.23%.

:exclamation: Current head 15c41a7 differs from pull request most recent head e75519d. Consider uploading reports for the commit e75519d to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
- Coverage   98.88%   98.83%   -0.06%     
==========================================
  Files          38       38              
  Lines        1441     1453      +12     
==========================================
+ Hits         1425     1436      +11     
- Misses         16       17       +1     
Flag Coverage Δ
unittests 98.83% <95.23%> (-0.06%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
reciprocalspaceship/dataset.py 98.82% <95.23%> (-0.21%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9dc62ba...e75519d. Read the comment docs.

JBGreisman commented 3 years ago

My most recent push added a test that uses fmodel data to compute a real-space map, shifts the map based on a desired translation using np.roll, and then back computes corresponding complex structure factors. These are compared to the results of DataSet.apply_symop().

JBGreisman commented 3 years ago

I am still working on this for the complex structure factors. This test may not be working as well as I had hoped, but I am not yet ready to come to that conclusion

JBGreisman commented 3 years ago

This PR now fixes DataSet.apply_symop() for use with both |F|+phi and complex SFs. I updated the test to use a P1 dataset avoid an interdependency caused by the use of apply_symop() in DataSet.to_reciprocalgrid().

P.S. -- I updated my emacs recently to auto-apply black formatting, which is why there are additional changes to the test_dataset_symops.py file