Nisar feature branch v11: extended csxtools to AXIS detector data #98
Nisar feature branch v11: extended csxtools to AXIS detector data #98ambarb merged 48 commits intoNSLS-II-CSX:masterfrom
Conversation
…this (csxtools/axis1/__init__.py) is fixed
…to include roation by 90 degree for AXIS part
ambarb
left a comment
There was a problem hiding this comment.
@nisarnk I've finished reviewing.
there are a few things worth discussing, but most of the changes I am requisition is the uniform use of axis not axis1 (or AXIS not AXIS1)
I'll make some additional issues to so that I can address things that are leftover from helpers
…tectors and to modify default behaviour
Got it! Here's a concise PR comment explaining why the three setuptools-related imports are used:
|
Regarding the change from 'axis1' to 'axis', this is a straightforward but sensitive change. It needs to be made consistently across several files in the package, and if not done carefully, it can lead to unexpected results or bugs due to conflicting names in the namespace. That’s why I considered it a lower-priority issue. The public APIs are already correctly named (e.g., from csxtools import get_axis_images, get_axis_flatfield), and the use of 'axis1' is hidden from the user. |
|
We have made separate GitHub issues to address the items that need to be fixed in subsequent PRs. This one is getting too complex. All code works, either raising errors or completing as expected, just some affordance issues need to be addressed.
Additionally, everything in There is 1 unresolved code comment about the @maffettone @thomashopkins32 ready for one of you to review. |
maffettone
left a comment
There was a problem hiding this comment.
Approve with some agita.
- In general, it is best practice to separate
MAINTPRs fromFEATPRs. This way, a reviewer is only looking at changes that make a difference. Looking through the 36 files changed, many of them are just formatting fixes, which can be largely accepted at face value. This means MAINT can go quickly through, while the followup FEAT PR can get a more detailed look. Not a blocker, just a note for the future :). - I'm not here to yuck anyone's yums, but am a little concerned about the injection of C code to perform optimized array optimizations (when this may already by accomplished by Dask or other libraries). It makes maintenance a bit more challenging. Again, I won't consider that a blocker, just an opinion.
- I'm admittedly not an expert with Python C bindings, so I will accept the extra components at face value. Had a look over the array math and that LGTM.
Remove Travis CI badge (inactive) and replace with GitHub Actions test badge
Summary
This PR extends the
csxtoolspackage to support data collected using the AXIS detector and renames/adds functions for improved clarity and performance.Changes
Added new functions for AXIS detector support:
get_axis_imagesget_axis_flatfieldget_axis_timestampsRefactored part of the AXIS image correction pipeline:
rotate90) are now handled by the C extension module.Updated
__init__.pyto reflect the new function names.Added backward-compatible aliases to avoid breaking existing code.
Testing