-
Notifications
You must be signed in to change notification settings - Fork 13
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
Physics package #1710
base: master
Are you sure you want to change the base?
Physics package #1710
Conversation
Hello @bnmajor! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-09-06 21:52:01 UTC |
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@kitware.com>
6a27124
to
509cb78
Compare
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
5c7c1ec
to
20f5c79
Compare
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
The default values are correct for TARDIS. Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
20f5c79
to
b4c4be4
Compare
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
0acc766
to
343f64b
Compare
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
343f64b
to
689886d
Compare
@psavery @saransh13 Some additional questions:
|
My thoughts are:
|
Looks like this only can guess TARDIS, or am I missing something? If not I can add PXRDIP |
Only expect additional material files for window and pinhole. Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
96eca6c
to
4033a97
Compare
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
4033a97
to
4b707ed
Compare
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
Signed-off-by: Brianna Major <brianna.major@taloid.khq.kitware.com>
b1b8835
to
4be85f7
Compare
<property name="styleSheet"> | ||
<string notr="true">QDoubleSpinBox:disabled {background-color: LightGray;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -143,10 +143,11 @@ def serialize(self): | |||
|
|||
@classmethod | |||
def deserialize(cls, d): | |||
from hexrdgui.hexrd_config import HexrdConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
self.ui.button_box.accepted.connect(self.accept_changes) | ||
self.ui.button_box.accepted.connect(self.ui.accept) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
from hexrdgui import resource_loader | ||
from hexrdgui.hexrd_config import HexrdConfig | ||
from hexrdgui.ui_loader import UiLoader | ||
import hexrdgui.resources.materials as module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import hexrdgui.resources.materials as module | |
import hexrdgui.resources.materials as materials_resources |
Can you rename this to something more specific than module
?
|
||
absorption_length = self.get_rygg_absorption_length(i) | ||
if np.isclose(value, absorption_length): | ||
if name == HexrdConfig().pinhole_package.material: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Relies on the HEXRD absorption-correction branch.