Physics package #1710

bnmajor · 2024-07-02T14:12:55Z

Relies on the HEXRD absorption-correction branch.

pep8speaks · 2024-07-02T14:13:03Z

Hello @bnmajor! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file hexrdgui/absorption_correction_options_dialog.py:

Line 71:80: E501 line too long (83 > 79 characters)

In the file hexrdgui/hexrd_config.py:

Line 529:80: E501 line too long (89 > 79 characters)

In the file hexrdgui/main_window.py:

Line 1666:80: E501 line too long (82 > 79 characters)
Line 1667:80: E501 line too long (82 > 79 characters)

In the file hexrdgui/physics_package_manager_dialog.py:

Line 76:80: E501 line too long (83 > 79 characters)
Line 200:80: E501 line too long (81 > 79 characters)

In the file hexrdgui/pinhole_correction_editor.py:

Line 157:80: E501 line too long (86 > 79 characters)

Comment last updated at 2024-09-27 00:43:31 UTC

bnmajor · 2024-09-02T13:05:28Z

@psavery @saransh13 Some additional questions:

We currently expect there to be a sample_materials and window_materials file like the pinhole_materials which we do not have - do we need to create these files (and if so which materials should be included)? Or should we just use the pinhole_materials file?
1. Same question for filter/coating/phosphor for the absorption correction dialog
We discussed automatically applying pinhole corrections and that the pinhole should be set to None if it is not needed but that is not currently an option.
1. Do we want to change this condition to allow None as well? If so, is it safe to assume that we will never call calc_transmission (which calls calc_effective_pinhole_area) without a pinhole?
2. Would it be simpler to say that if this is a TARDIS or PXRDIP (and eventually BBXRD) instrument we just automatically apply pinhole corrections?
How should we handle inferring instrument type? Currently we set that automatically if users come from the LLNL import tool, but how should we handle this if they do not? We have engineering constraints to determine if it is TARDIS, but no clear solution for PXRDIP.
Absorption correction is being normalized but still looked like it may be incorrect - @saransh13 I will need your feedback here for sure

psavery · 2024-09-04T19:08:14Z

My thoughts are:

Yes we probably need to make those files (but I think filter/coating/phosphor will just be a dict within HEXRD)
I think we probably want to allow the transmission to be calculated without a pinhole (make the pinhole optional in transmission calculation)
You can just use this function, Brianna. That detects both TARDIS and PXRDIP automatically (and only those two so far) based upon the detector names.
It looks like we are finding the max transmission across all detectors and normalizing based upon that. I think that is the behavior we planned. Maybe something else is missing, @saransh13?

bnmajor · 2024-09-04T21:40:55Z

You can just use this function, Brianna. That detects both TARDIS and PXRDIP automatically (and only those two so far) based upon the detector names.

Looks like this only can guess TARDIS, or am I missing something? If not I can add PXRDIP

psavery · 2024-09-06T18:17:29Z

hexrdgui/resources/ui/pinhole_mask_dialog.ui

+     <property name="styleSheet">
+      <string notr="true">QDoubleSpinBox:disabled {background-color: LightGray;}


I've found it usually better to not edit the style sheets (when possible), because when we do so, things like dark mode on the Mac don't change colors properly.

psavery · 2024-09-06T18:20:18Z

hexrdgui/polar_distortion_object.py

@@ -143,10 +143,11 @@ def serialize(self):

    @classmethod
    def deserialize(cls, d):
+        from hexrdgui.hexrd_config import HexrdConfig


Can you move this import down into the if statement right before it is used? That will help us to not import it unless we actually need it...

This could alternatively be imported at the top of the file, but I'm assuming there's a circular import issue or something like that.

psavery · 2024-09-06T18:23:40Z

hexrdgui/pinhole_panel_buffer.py

@@ -11,7 +11,8 @@ def generate_pinhole_panel_buffer(instr, pinhole_radius, pinhole_thickness):
    instr.beam_vector = np.r_[0., 0., -1.]
    try:
        for det_key, det in instr.detectors.items():
-            crit_angle = np.arctan(2 * pinhole_radius / pinhole_thickness)
+            crit_angle = np.arctan(
+                det.pinhole.diameter / det.pinhole.thickness)


I really think the pinhole ought to be a property of the instrument rather than the detector (there is only one pinhole per experiment, there are not separate pinholes for separate detectors).

Can we change HEXRD to have the following be on the instrument: sample layer, window, pinhole. If the detector needs it for something, maybe we can pass them as arguments?

Let me know if that is a lot of work to do.

psavery · 2024-09-06T18:27:44Z

hexrdgui/physics_package_manager_dialog.py

+        self.ui.button_box.rejected.connect(self.ui.reject)
+        for k, w in self.material_selectors.items():
+            w.currentIndexChanged.connect(
+                lambda index, k=k: self.material_changed(index, k))


I've sometimes ran into trouble using loop variables in lambda expressions. Maybe this is fine - but can you just verify that all values of k (not just the last one) work properly?

psavery · 2024-09-06T18:28:59Z

hexrdgui/physics_package_manager_dialog.py

+        self.ui.button_box.accepted.connect(self.accept_changes)
+        self.ui.button_box.accepted.connect(self.ui.accept)


I usually think it's better to just connect to a signal once, and have an expression like on_accepted() that calls self.accept_changes() and self.ui.accept().

However, you don't need to change this! That's just FYI.

psavery · 2024-09-06T20:54:13Z

hexrdgui/absorption_correction_options_dialog.py

+from hexrdgui import resource_loader
+from hexrdgui.hexrd_config import HexrdConfig
+from hexrdgui.ui_loader import UiLoader
+import hexrdgui.resources.materials as module


Suggested change

import hexrdgui.resources.materials as module

import hexrdgui.resources.materials as materials_resources

Can you rename this to something more specific than module?

psavery · 2024-09-06T22:00:41Z

hexrdgui/pinhole_correction_editor.py

-
-            absorption_length = self.get_rygg_absorption_length(i)
-            if np.isclose(value, absorption_length):
+            if name == HexrdConfig().pinhole_package.material:


Are we still letting users manually edit the absorption correction length in the Rygg pinhole correction editor? If not, I don't think we need this, right?

Signed-off-by: Brianna Major <[email protected]>

Only expect additional material files for window and pinhole. Signed-off-by: Brianna Major <[email protected]>

Signed-off-by: Brianna Major <[email protected]>

These do not work well on Mac Signed-off-by: Brianna Major <[email protected]>

Signed-off-by: Brianna Major <[email protected]>

Not all instruments will need a physics package or detector coating information, so we shouldn't save this information to the instrument. This will prevent writing out extra information to the config file for users that do not want or expect it. Signed-off-by: Brianna Major <[email protected]>

Signed-off-by: Brianna Major <[email protected]>

The hexrd classes still expect a pinhole radius. Use that in the check. Signed-off-by: Patrick Avery <[email protected]>

Otherwise, there are errors... Signed-off-by: Patrick Avery <[email protected]>

We are running into trouble using it for other systems. For now, disable them. We'll add them back in later. Signed-off-by: Patrick Avery <[email protected]>

bnmajor force-pushed the physics-package branch 5 times, most recently from 6a27124 to 509cb78 Compare July 3, 2024 19:55

bnmajor force-pushed the physics-package branch from 509cb78 to d10910f Compare July 11, 2024 20:00

bnmajor force-pushed the physics-package branch 2 times, most recently from 5c7c1ec to 20f5c79 Compare August 14, 2024 20:40

bnmajor force-pushed the physics-package branch 3 times, most recently from 343f64b to 689886d Compare September 2, 2024 12:28

bnmajor force-pushed the physics-package branch 3 times, most recently from b1b8835 to 4be85f7 Compare September 6, 2024 21:51

psavery reviewed Sep 6, 2024

View reviewed changes

bnmajor force-pushed the physics-package branch 3 times, most recently from e7e50f6 to 0172e05 Compare September 20, 2024 13:36

bnmajor and others added 8 commits September 26, 2024 19:13

Add the initial physics package manage dialog

8576816

Signed-off-by: Brianna Major <[email protected]>

Move forms into tab widget

6bef1ab

Signed-off-by: Brianna Major <[email protected]>

Automaically show physics package manager after LLNL import

b6a1f60

Signed-off-by: Brianna Major <[email protected]>

Include default materials

5b7bf1c

Signed-off-by: Brianna Major <[email protected]>

Use default values

063f723

Signed-off-by: Brianna Major <[email protected]>

Save changes to all detectors on accept

16d0ff1

Signed-off-by: Brianna Major <[email protected]>

Add selector for physics package type

d2ced9b

Signed-off-by: Brianna Major <[email protected]>

Add option to use physics package to intensity corrections

5d4d12d

Signed-off-by: Brianna Major <[email protected]>

Brianna Major and others added 23 commits September 26, 2024 19:13

Add window_materials file

c569a37

Only expect additional material files for window and pinhole. Signed-off-by: Brianna Major <[email protected]>

Guess instrument type from detectors

c264aa5

Signed-off-by: Brianna Major <[email protected]>

Fix package type default

3da9453

Signed-off-by: Brianna Major <[email protected]>

WIP: Get filter/coating/phosphor defaults

dd335d1

Signed-off-by: Brianna Major <[email protected]>

Perform subtract minimum last

6eefdd2

Signed-off-by: Brianna Major <[email protected]>

Correct absorption correction editor title

91f915c

Signed-off-by: Brianna Major <[email protected]>

Clear material input when not set to "enter manually"

e4be0a5

Signed-off-by: Brianna Major <[email protected]>

Add phosphor units

9a419fe

Signed-off-by: Brianna Major <[email protected]>

Use recently added return values instead of attribute

6c652ba

Signed-off-by: Brianna Major <[email protected]>

Use pinhole info from physics package

934bd7d

Signed-off-by: Brianna Major <[email protected]>

Use update functions rather than setters

460933c

Signed-off-by: Brianna Major <[email protected]>

Physics package is now at instrument level

b2ca011

Signed-off-by: Brianna Major <[email protected]>

Update default values on instrument change

60af650

Signed-off-by: Brianna Major <[email protected]>

Only import config if needed

c86da4f

Signed-off-by: Brianna Major <[email protected]>

Do not make stylesheet changes

e3aee3d

These do not work well on Mac Signed-off-by: Brianna Major <[email protected]>

Remove unused imports

456bb16

Signed-off-by: Brianna Major <[email protected]>

Remember absorption correction setting in state

16e047c

Signed-off-by: Brianna Major <[email protected]>

Save physics package and detector coatings to state

d5bbfe1

Signed-off-by: Brianna Major <[email protected]>

Import materials from hexrd resources rather than hexrdgui

2811a71

Signed-off-by: Brianna Major <[email protected]>

Support instruments with no default physics package

0f3a05a

Signed-off-by: Brianna Major <[email protected]>

Go back to pinhole radius for hexrd classes

68939b3

The hexrd classes still expect a pinhole radius. Use that in the check. Signed-off-by: Patrick Avery <[email protected]>

Fix detector coatings to accept 'None' from state

b407b04

Otherwise, there are errors... Signed-off-by: Patrick Avery <[email protected]>

psavery force-pushed the physics-package branch from 69cfea4 to b407b04 Compare September 27, 2024 00:26

psavery marked this pull request as ready for review September 27, 2024 00:41

psavery requested a review from saransh13 September 27, 2024 00:41

Physics package is now TARDIS/PXRDIP only

eaffc68

We are running into trouble using it for other systems. For now, disable them. We'll add them back in later. Signed-off-by: Patrick Avery <[email protected]>

psavery force-pushed the physics-package branch from ce1ad50 to eaffc68 Compare September 27, 2024 00:43

saransh13 approved these changes Sep 27, 2024

View reviewed changes

saransh13 merged commit e0df2b8 into HEXRD:master Sep 27, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Physics package #1710

Physics package #1710

bnmajor commented Jul 2, 2024 •

edited

Loading

pep8speaks commented Jul 2, 2024 •

edited

Loading

bnmajor commented Sep 2, 2024 •

edited

Loading

psavery commented Sep 4, 2024

bnmajor commented Sep 4, 2024

psavery Sep 6, 2024

psavery Sep 6, 2024

psavery Sep 6, 2024

psavery Sep 6, 2024

psavery Sep 6, 2024

psavery Sep 6, 2024

psavery Sep 6, 2024

		<property name="styleSheet">
		<string notr="true">QDoubleSpinBox:disabled {background-color: LightGray;}

		self.ui.button_box.accepted.connect(self.accept_changes)
		self.ui.button_box.accepted.connect(self.ui.accept)

	import hexrdgui.resources.materials as module
	import hexrdgui.resources.materials as materials_resources

Physics package #1710

Physics package #1710

Conversation

bnmajor commented Jul 2, 2024 • edited Loading

pep8speaks commented Jul 2, 2024 • edited Loading

Comment last updated at 2024-09-27 00:43:31 UTC

bnmajor commented Sep 2, 2024 • edited Loading

psavery commented Sep 4, 2024

bnmajor commented Sep 4, 2024

psavery Sep 6, 2024

Choose a reason for hiding this comment

psavery Sep 6, 2024

Choose a reason for hiding this comment

psavery Sep 6, 2024

Choose a reason for hiding this comment

psavery Sep 6, 2024

Choose a reason for hiding this comment

psavery Sep 6, 2024

Choose a reason for hiding this comment

psavery Sep 6, 2024

Choose a reason for hiding this comment

psavery Sep 6, 2024

Choose a reason for hiding this comment

bnmajor commented Jul 2, 2024 •

edited

Loading

pep8speaks commented Jul 2, 2024 •

edited

Loading

bnmajor commented Sep 2, 2024 •

edited

Loading