Advanced Options for Single Crystal Elastic mode of DNS Reduction GUI #37915

koshchii · 2024-09-04T11:08:45Z

Description of work

This PR contains advanced data reduction options selection capabilities for the Single Cristal Elastic mode of DNS Reduction GUI. Complete integration of the Single Cristal Elastic mode into the DNS Reduction GUI will be implemented in several steps (in order to ensure reasonable PR sizes and reduce the workload for the reviewers), and this PR represents the 5th step of the integration process.

Summary of work

Fixes Part 5 of #36407, as well as a bug reported in #37712.

To test:

Download test data: Single_Crystal_Elastic.zip
Open the reduction GUI [Interfaces->Direct->DNS Reduction]
Make sure that the "Single Crystal Elastic" mode opens by default. Otherwise switch to this mode, by selecting Tools -> Change Mode -> Single Crystal Elastic
Make sure that the corresponding documentation is up-to-date (run cmake --build . --target docs-html in build folder to create docs)
Browse to the directory where the test data are located using the "Paths" tab of the GUI
Load and select the sample data for reduction using the "Data" tab
Click on the "Options" tab and and make sure that various option selection fields work as expected, and the selected options are then applied to the data once the data reduction script is generated using the "Script Generator" tab
Reduce the data and generate a data reduction script using the "Script Generator" tab
Open the "Plotting" tab and make sure that the images are generated. Advanced plotting functionality is not implemented at this stage and will be activated in a follow-up PR.

This does not require release notes because this PR contains limited functionality. Release notes for the Single Cristal Elastic mode will be included in a subsequent PR, once all components for implementing the data reduction workflow are uploaded.

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.

Paraquat

General observations:

If I click on the Generate script button under the Script generator tab, then paste the script into the main window of Mantid, then run it, the data visualised under the Plotting tab is no longer meaningful.
This button should be labelled Generate and run script. I did not realise that it was actually running the script.
If I don't click on the Options tab during the workflow, I get an arcane message about not setting the binning, when all I needed to do was click on the Options tab. This does not seem like user-friendly behaviour.
This widget is a bit counter-intuitive. If I edit 2theta min the hit tab, I'm taken to the omega min box instead of 2theta max. This may just be a personal thing, but I feel like the the 2theta controls should be in a separate widget from the omega controls because it's a little difficult to visually parse.

Paraquat · 2024-09-05T05:14:30Z

...ython/mantidqtinterfaces/mantidqtinterfaces/dns_single_crystal_elastic/helpers/converters.py

+    h, k, l = hkl
+    alpha = radians(alpha)
+    beta = radians(beta)
+    gamma = radians(gamma)
+    cos_alpha = cos(alpha)
+    cos_beta = cos(beta)
+    cos_gamma = cos(gamma)
+    sin_alpha = sin(alpha)
+    sin_beta = sin(beta)
+    sin_gamma = sin(gamma)
+    vol = a * b * c * sqrt(1.0 - cos_alpha**2 - cos_beta**2 - cos_gamma**2 + 2.0 * cos_alpha * cos_beta * cos_gamma)
+    s11 = b**2 * c**2 * sin_alpha**2
+    s22 = a**2 * c**2 * sin_beta**2
+    s33 = a**2 * b**2 * sin_gamma**2
+    s12 = a * b * c**2 * (cos_alpha * cos_beta - cos_gamma)
+    s23 = a**2 * b * c * (cos_beta * cos_gamma - cos_alpha)
+    s13 = a * b**2 * c * (cos_gamma * cos_alpha - cos_beta)
+    inverse_d_square = 1.0 / vol**2 * (s11 * h**2 + s22 * k**2 + s33 * l**2 + 2.0 * s12 * h * k + 2 * s23 * k * l + 2 * s13 * h * l)
+    d = sqrt(1.0 / inverse_d_square)


This can be reduced to 2 lines of linear algebra assuming you have access to the reciprocal basis b, with maybe 1 extra function call to convert the lattice parameters to a matrix. I'm sure that these functions must exist somewhere in Mantid.

q = (hkl) x b^T (vector hkl multiplied by transpose reciprocal basis)
d = 1 / |q|

This would be much less error prone than reproducing all of the equations in terms of angles.

Nice suggestion, thanks! It turns out that the UnitCell class in mantid.geometry has a method to calculate d out of hkl and lattice parameters. I updated this part correspondingly.

Paraquat · 2024-09-05T05:50:04Z

...antidqtinterfaces/dns_single_crystal_elastic/options/elastic_single_crystal_options_model.py

+    def convert_hkl_string(hkl):
+        # catches if user uses brackets or spaces in hkl specification
+        hkl = hkl.strip("[]()")
+        hkl = hkl.replace(" ", ",")
+        return hkl


Instead of ensuring that the strings passed to the core function are correct, you could just not allow the user to enter anything other than 3 integers (e.g. 3 QSpinBox widgets). This may not be appropriate if you forsee the user having to enter a lot of (hkl) triples.

Good point, thanks. I decided to keep the input as a string, but included a QRegExpValidator in elastic_single_crystal_options_view.py that makes sure that only allowed pattern can be entered

Paraquat · 2024-09-05T06:03:48Z

...dqtinterfaces/dns_single_crystal_elastic/options/elastic_single_crystal_options_presenter.py

+        binning_param_dict = {}
+        binning_param_dict["two_theta"] = get_automatic_two_theta_binning(sample_data)
+        binning_param_dict["omega"] = get_automatic_omega_binning(sample_data)


The name binning_param_dict seems a bit misleading since it contains histogram bin edges, which I would consider to be data rather than parameters.

Thanks! I've refactored it

Paraquat · 2024-09-05T06:04:32Z

...dqtinterfaces/dns_single_crystal_elastic/options/elastic_single_crystal_options_presenter.py

+            for parameter in binning_param_dict[key].keys():
+                own_options[parameter] = binning_param_dict[key][parameter]
+
+    def _set_min_max_binning_lims(self, sample_data):


min and max make this name unneccessarily long (they are implied by the lim part)

thanks, I've refactored it

Paraquat · 2024-09-06T12:26:48Z

...terfaces/mantidqtinterfaces/dns_powder_elastic/data_structures/dns_elastic_powder_dataset.py

                fields.append(field)
-        angle_and_fields_data[det_bank] = set(fields)
-    return angle_and_fields_data
+        unique_fields = list(set(fields))


What data type does fields contain? Does this type have a reasonable equality operator for the purposes of set?

not sure if I understand what you mean here. fields is a list of strings, and I used set() to keep only unique values (drop duplicates)

Paraquat · 2024-09-06T12:29:03Z

...hon/mantidqtinterfaces/mantidqtinterfaces/dns_powder_tof/file_selector/file_selector_view.py

+        self._treeview.blockSignals(True)
        self._treeview.collapseAll()
+        self._treeview.blockSignals(False)


I think the best practice is to use a scoped QSignalBlocker rather than a pair of calls to blockSignals.

thanks, I improved this part

Paraquat · 2024-09-06T13:19:58Z

...nterfaces/mantidqtinterfaces/dns_single_crystal_elastic/scripts/md_single_crystal_elastic.py

+                    try:
+                        vana = mtd[vana_name]
+                        vana_norm = mtd[vana_norm]
+                    except KeyError:
+                        raise_error(f"No vanadium file for field {field_name}.")
+                        return mtd[workspace_name]


I prefer something like dict.get(some_key, some_default) to using exceptions as flow control. some_default is by default None, so you can also use if dict.get(some_key)...`.

this won't work here because mtd is not a dictionary, but rather a call to a workspace available (or not) in the list of created workspaces (see https://archive.mantidproject.org/Accessing_Workspaces_From_Python for ref). I modified the code using mtd.doesExist() so it works in a similar way you're suggesting, i.e. using if...else instead

Paraquat · 2024-09-10T14:01:22Z

...aces/mantidqtinterfaces/dns_single_crystal_elastic/options/elastic_single_crystal_options.ui

@@ -645,7 +645,7 @@
             <bool>false</bool>
            </property>
            <property name="maximum">
-             <double>150.000000000000000</double>


Is there a reason for this level of numerical precision in this (xml?) file?

not really, I just updated the value using QtDesigner, and for some reason that is a default format there

Paraquat · 2024-09-10T14:03:58Z

...dqtinterfaces/dns_single_crystal_elastic/options/elastic_single_crystal_options_presenter.py

-            self._set_auto_omega_binning()
+        selected_data = self.param_dict["file_selector"]["full_data"]
+        if selected_data:
+            get_wavelength_from_data_is_on = self._get_wavelength_from_data_state()


No real need to define this variable (see below, also). Why not just call the function directly from the if statement. If the issue is clarity, then why not rename the function to the name of this variable?

I updated the code correspondingly

Paraquat · 2024-09-10T14:54:38Z

...nterfaces/mantidqtinterfaces/dns_single_crystal_elastic/scripts/md_single_crystal_elastic.py

+    if not standard:
+        LoadDNSSCD(
+            FileNames=filepaths,
+            OutputWorkspace=workspace_name,
+            NormalizationWorkspace=norm_name,
+            Normalization=params["norm_to"],
+            a=params["a"],
+            b=params["b"],
+            c=params["c"],
+            alpha=params["alpha"],
+            beta=params["beta"],
+            gamma=params["gamma"],
+            OmegaOffset=params["omega_offset"],
+            HKL1=params["hkl1"],
+            HKL2=params["hkl2"],
+            LoadAs="raw",
+            SaveHuberTo=f"huber_{field_name}",
+        )
+    else:
+        try:
+            LoadDNSSCD(
+                FileNames=filepaths,
+                OutputWorkspace=workspace_name,
+                NormalizationWorkspace=norm_name,
+                Normalization=params["norm_to"],
+                a=params["a"],
+                b=params["b"],
+                c=params["c"],
+                alpha=params["alpha"],
+                beta=params["beta"],
+                gamma=params["gamma"],
+                OmegaOffset=params["omega_offset"],
+                HKL1=params["hkl1"],
+                HKL2=params["hkl2"],
+                LoadAs="raw",
+                LoadHuberFrom=f"huber_{field_name}",
+            )
+        # handling of std data when sample and vana fields are different but ignore
+        # vanadium fields option is selected
+        except ValueError:
+            if params["ignore_vana"]:
+                for field in sample_fields_to_try:
+                    try:
+                        LoadDNSSCD(
+                            FileNames=filepaths,
+                            OutputWorkspace=workspace_name,
+                            NormalizationWorkspace=norm_name,
+                            Normalization=params["norm_to"],
+                            a=params["a"],
+                            b=params["b"],
+                            c=params["c"],
+                            alpha=params["alpha"],
+                            beta=params["beta"],
+                            gamma=params["gamma"],
+                            OmegaOffset=params["omega_offset"],
+                            HKL1=params["hkl1"],
+                            HKL2=params["hkl2"],
+                            LoadAs="raw",
+                            LoadHuberFrom=f"huber_{field}",
+                        )


Some boilerplate here. It's very difficult to tell the difference between these three calls of the same function. Is it possible to handle these 3 cases in the LoadDNSSCD constructor? You could pass standard as an argument, and then catch any exceptions in the class constructor.

I added process_dns_scd_raw function, which has standard as an argument, to handle these cases. I also removed the unnecessary declaration of lattice properties in a call of LoadDNSSCD, since these properties are not used anyways when LoadAs parameter is set to raw.

koshchii · 2024-10-25T02:47:52Z

Comments to the observations with screenshots:

1. If I click on the `Generate script` button under the `Script generator` tab, then paste the script into the main window of Mantid, then run it, the data visualised under the `Plotting` tab is no longer meaningful.

This is, in principle, not the way how a user is expected to run the workflow, but this behavior should be fixed in future. I added a corresponding to-do-item in issue Workflow handling improvements of DNS_Reduction GUI #36064.

2. This button should be labelled `Generate and run script`. I did not realise that it was actually running the script.

Thanks, that's fixed now.

3. If I don't click on the `Options` tab during the workflow, I get an arcane message about not setting the binning, when all I needed to do was click on the `Options` tab. This does not seem like user-friendly behaviour.

This behavior should be investigated and an optimal solution should be found and implemented. I added a corresponding to-do-item in issue Workflow handling improvements of DNS_Reduction GUI #36064.

4. This widget is a bit counter-intuitive. If I edit `2theta min` the hit tab, I'm taken to the `omega min` box instead of `2theta max`. This may just be a personal thing, but I feel like the the 2theta controls should be in a separate widget from the omega controls because it's a little difficult to visually parse.

Thanks, I think it makes sense. It is fixed now.

koshchii added 4 commits September 4, 2024 11:19

Add advanced data reduction options selection capabilities

51241d3

Improve binning-related options capabilities

8bff663

Fix bug with expand_all/unexpand_all buttons

2819f3c

Update unit tests

2ede503

koshchii added the MLZ Team Issue and pull requests managed by the MLZ development team label Sep 4, 2024

koshchii added this to the Release 6.11 milestone Sep 4, 2024

koshchii requested a review from Paraquat September 4, 2024 12:19

koshchii assigned Paraquat Sep 4, 2024

koshchii marked this pull request as ready for review September 4, 2024 12:21

koshchii mentioned this pull request Sep 4, 2024

Add Single Crystal Elastic mode to DNS Reduction GUI #36407

Open

7 tasks

koshchii modified the milestones: Release 6.11, Release 6.12 Sep 10, 2024

Paraquat reviewed Sep 10, 2024

View reviewed changes

Add changes from code review

66ff1a9

koshchii mentioned this pull request Oct 25, 2024

Workflow handling improvements of DNS_Reduction GUI #36064

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced Options for Single Crystal Elastic mode of DNS Reduction GUI #37915

Advanced Options for Single Crystal Elastic mode of DNS Reduction GUI #37915

koshchii commented Sep 4, 2024

Paraquat left a comment •

edited

Loading

Paraquat Sep 5, 2024

koshchii Oct 10, 2024

Paraquat Sep 5, 2024

koshchii Oct 4, 2024

Paraquat Sep 5, 2024

koshchii Oct 4, 2024

Paraquat Sep 5, 2024

koshchii Oct 4, 2024

Paraquat Sep 6, 2024

koshchii Oct 4, 2024

Paraquat Sep 6, 2024

koshchii Oct 4, 2024

Paraquat Sep 6, 2024

koshchii Oct 3, 2024

Paraquat Sep 10, 2024

koshchii Oct 4, 2024

Paraquat Sep 10, 2024

koshchii Oct 4, 2024

Paraquat Sep 10, 2024

koshchii Oct 10, 2024

koshchii commented Oct 25, 2024

Advanced Options for Single Crystal Elastic mode of DNS Reduction GUI #37915

Are you sure you want to change the base?

Advanced Options for Single Crystal Elastic mode of DNS Reduction GUI #37915

Conversation

koshchii commented Sep 4, 2024

Description of work

Summary of work

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

Paraquat left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koshchii commented Oct 25, 2024

Paraquat left a comment •

edited

Loading