Remove symmetry elements for SaveINS #38605

zjmorgan · 2025-01-15T03:59:14Z

Description of work

SaveINS incorrectly includes all symmetry elements and should not include identity, inversion, or any that can be generated by the lattice centering translations.

Summary of work

Redundant symmetry elements that can be generated by the underlying centering condition or rotoinversion should be removed. In addition, identity does not need to be included.

To test:

Run the additional system tests. Or, run this example. Choose any space group. Go to link https://cci.lbl.gov/cctbx/shelx.html and enter the same space group symbol. Cross check that it gives the same symmetry operators. Order does not matter.

# import mantid algorithms, numpy and matplotlib
from mantid.simpleapi import *
import matplotlib.pyplot as plt
import numpy as np

ws = CreateSampleWorkspace(OutputWorkspace='ws', NumBanks=1, BankPixelWidth=1, BinWidth=20000)  # 1 bin
SetUB(Workspace=ws, a=7.6508, b=13.2431, c=11.6243, alpha=90, beta=104.1183, gamma=90)
ndensity = 4 / ws.sample().getOrientedLattice().volume()  # number density with 4 formula units per u.c
SetSample(InputWorkspace=ws, Material={'ChemicalFormula': 'C12 H9 N3 O2 S1',
          'SampleNumberDensity': ndensity})
SaveINS(InputWorkspace=ws, Filename='/tmp/SHELX.ins', Spacegroup='I a -3 d', UseNaturalIsotopicAbundances=True)

Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

Is the code of an acceptable quality?
Does the code conform to the coding standards?
Are the unit tests small and test the class in isolation?
If there is GUI work does it follow the GUI standards?
If there are changes in the release notes then do they describe the changes appropriately?
Do the release notes conform to the release notes guide?

Functional Tests

Do changes function as described? Add comments below that describe the tests performed?
Do the changes handle unexpected situations, e.g. bad input?
Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

sf1919 · 2025-01-15T10:44:08Z

Can we get a milestone on this so we know whether we're trying to get it into v6.12 or not?

thomashampson · 2025-01-15T13:40:46Z

docs/source/release/6.12.0/Diffraction/Single_Crystal/38605.rst

@@ -0,0 +1 @@
+- Fix :ref:`SaveINS<algm-SaveINS-v1>` that saved all symmetry records to file. Only the minimum are needed that can be generated by translation/rotation corresponding to the lattice type.


I think this needs to move a level deeper, into the Bugfixes folder

RichardWaiteSTFC

Thanks for this @zjmorgan - I wasn't aware of this specification from SHELX, or the online tool you link in the PR. Just some small suggestions!

RichardWaiteSTFC · 2025-01-17T10:41:42Z

Framework/PythonInterface/plugins/algorithms/SaveINS.py

+
+    def _symmetry_matrix_vector(self, symop):
+        """Symmetry rotation matrix and translation vector"""
+        W = np.linalg.inv(np.vstack([symop.transformHKL(V3D(*vec)) for vec in np.eye(3)]))


Minor, but could have more informative variable names throughout - it looks like W is the symmetry operators transformation matrix (which you'd think we should have a getter for on the C++ side) and wis the translation part?

Good suggestion. I changed the names to W_mat and w_vec

RichardWaiteSTFC · 2025-01-17T10:42:54Z

Framework/PythonInterface/plugins/algorithms/SaveINS.py

+            S1 = self._symmetry_operation_key(W1, w1)
+            if S1 not in latt_type_ops_set:
+                # identity(/inversion) rotation symmetry operator
+                for rot in self.ROTATION_OPS[latt_sign]:


Seems like this is a repeat of earlier just with sym_op instead of rot (is that correct?)

mantid/Framework/PythonInterface/plugins/algorithms/SaveINS.py

Lines 195 to 201 in 80dd7f9

for rot in self.ROTATION_OPS[latt_sign]:

W2, _ = self._symmetry_matrix_vector(rot)

# lattice centering translation symmetry operator

for cent in self.CENTERING_OPS[latt_numb]:

_, w2 = self._symmetry_matrix_vector(cent)

# equivalent symmetry operator generated from rotation and translation

S3 = self._symmetry_operation_key(np.eye(3), np.zeros(3), W2, w2)

Could it be refactored into a separate function?

I re-organized the code a bit so it is easier to follow.

RichardWaiteSTFC · 2025-01-17T10:48:47Z

Framework/PythonInterface/plugins/algorithms/SaveINS.py

+                            W_dict[S3] = W1
+                            w_dict[S3] = w1
+                        # bias toward identity
+                        if np.linalg.det(W1) > np.linalg.det(W_dict[S3]):


Why is this check necessary - are the rotations and translations not equivalent to what is already in the dictionary?

Even though they are equivalent, I bias toward the operation with simpler notation

"-x+y,-x,z" is simpler than "-x+y+1/3,-x+2/3,z+2/3"

RichardWaiteSTFC · 2025-01-17T10:59:20Z

Framework/PythonInterface/plugins/algorithms/SaveINS.py

@@ -140,5 +173,67 @@ def PyExec(self):
            f_handle.write("HKLF 2\n")  # tells SHELX the columns saved in the reflection file
            f_handle.write("END")

+    def _symmetry_operation_key(self, W1, w1, W2=np.eye(3), w2=np.zeros(3)):
+        """Symmetry element key (9 element tuple) for comparison"""


Perhaps a bit more explanation as to why this works as a unique identifier?

zjmorgan · 2025-01-17T11:38:19Z

Thanks for this @zjmorgan - I wasn't aware of this specification from SHELX, or the online tool you link in the PR. Just some small suggestions!

Me either! Here is what I found /https://macxray.chem.upenn.edu/LATT.pdf

Remove symmetry elements

d217e7a

Forgot primitive

703a22c

zjmorgan added Diffraction Issues and pull requests related to diffraction Single Crystal Issues and pull requests related to single crystal Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) labels Jan 15, 2025

zjmorgan added this to the Release 6.12 milestone Jan 15, 2025

zjmorgan added 5 commits January 15, 2025 06:43

Comment code

076e038

Adding release doc

88b7bc2

Fix typo

1b50fe7

C-centering

127ad8a

A/B/C-centering

3f6b975

thomashampson reviewed Jan 15, 2025

View reviewed changes

zjmorgan added 3 commits January 15, 2025 09:00

Move release note

72ffabb

Add indentity back

b9697cf

Bias toward right-handed systems

f499fea

jclarkeSTFC changed the base branch from main to release-next January 16, 2025 15:40

zjmorgan added 2 commits January 16, 2025 11:57

Bias towards identity and origin

9a1ddec

Add new tests for SaveINS

80dd7f9

zjmorgan requested a review from RichardWaiteSTFC January 16, 2025 22:31

RichardWaiteSTFC self-assigned this Jan 17, 2025

RichardWaiteSTFC requested changes Jan 17, 2025

View reviewed changes

zjmorgan added 3 commits January 17, 2025 06:25

Re-organize and comment

a227a36

Re-organize and comment

aa64c8d

Re-organize and comment

af956a0

Fix typo in comment

12516df

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove symmetry elements for SaveINS #38605

Remove symmetry elements for SaveINS #38605

zjmorgan commented Jan 15, 2025 •

edited

Loading

sf1919 commented Jan 15, 2025

thomashampson Jan 15, 2025

RichardWaiteSTFC left a comment

RichardWaiteSTFC Jan 17, 2025 •

edited

Loading

zjmorgan Jan 17, 2025

RichardWaiteSTFC Jan 17, 2025 •

edited

Loading

zjmorgan Jan 17, 2025

RichardWaiteSTFC Jan 17, 2025

zjmorgan Jan 17, 2025

zjmorgan Jan 17, 2025

RichardWaiteSTFC Jan 17, 2025

zjmorgan commented Jan 17, 2025

		@@ -0,0 +1 @@
		- Fix :ref:`SaveINS<algm-SaveINS-v1>` that saved all symmetry records to file. Only the minimum are needed that can be generated by translation/rotation corresponding to the lattice type.

	for rot in self.ROTATION_OPS[latt_sign]:
	W2, _ = self._symmetry_matrix_vector(rot)
	# lattice centering translation symmetry operator
	for cent in self.CENTERING_OPS[latt_numb]:
	_, w2 = self._symmetry_matrix_vector(cent)
	# equivalent symmetry operator generated from rotation and translation
	S3 = self._symmetry_operation_key(np.eye(3), np.zeros(3), W2, w2)

Remove symmetry elements for SaveINS #38605

Are you sure you want to change the base?

Remove symmetry elements for SaveINS #38605

Conversation

zjmorgan commented Jan 15, 2025 • edited Loading

Description of work

Summary of work

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

sf1919 commented Jan 15, 2025

thomashampson Jan 15, 2025

Choose a reason for hiding this comment

RichardWaiteSTFC left a comment

Choose a reason for hiding this comment

RichardWaiteSTFC Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

zjmorgan Jan 17, 2025

Choose a reason for hiding this comment

RichardWaiteSTFC Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

zjmorgan Jan 17, 2025

Choose a reason for hiding this comment

RichardWaiteSTFC Jan 17, 2025

Choose a reason for hiding this comment

zjmorgan Jan 17, 2025

Choose a reason for hiding this comment

zjmorgan Jan 17, 2025

Choose a reason for hiding this comment

RichardWaiteSTFC Jan 17, 2025

Choose a reason for hiding this comment

zjmorgan commented Jan 17, 2025

zjmorgan commented Jan 15, 2025 •

edited

Loading

RichardWaiteSTFC Jan 17, 2025 •

edited

Loading

RichardWaiteSTFC Jan 17, 2025 •

edited

Loading