HEXRD / hexrdgui

Qt6 PySide6 based GUI for the HEXRD library.
Other
29 stars 14 forks source link

Physics package #1710

Closed bnmajor closed 1 month ago

bnmajor commented 4 months ago

Relies on the HEXRD absorption-correction branch.

pep8speaks commented 4 months ago

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

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

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

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

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

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

Comment last updated at 2024-09-27 00:43:31 UTC
bnmajor commented 2 months ago

@psavery @saransh13 Some additional questions:

  1. 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
  2. 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?
  3. 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.
  4. Absorption correction is being normalized but still looked like it may be incorrect - @saransh13 I will need your feedback here for sure
psavery commented 2 months ago

My thoughts are:

  1. Yes we probably need to make those files (but I think filter/coating/phosphor will just be a dict within HEXRD)
  2. I think we probably want to allow the transmission to be calculated without a pinhole (make the pinhole optional in transmission calculation)
  3. 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.
  4. 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 commented 2 months ago
  1. 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