Skip to content

Conversation

@pmneves7
Copy link

Changes:

  • Added Qz and dQz as optional value in 2D data
  • Created three_dim dataset type and test
  • Added deduce_qz to postprocess.py to create the Qz data in a dataset if wavelength, Qx, and Qy are provided.
  • Added rpm unit (also in pull request Added rpms as a unit #175)
  • Refactored temp_hdf5_reader.py to be more compliant with NXcanSAS standard. Now imports by canSAS_class instead of group name in hdf5 file.

Still to do:

paulneves77 and others added 11 commits November 13, 2025 14:15
…andards (now looks directly at canSAS_class instead of the group name. This is because the group name is allowed to be anything, but the canSAS_class is fixed. also allowed raw to be none in metadata because raw is not a part of the NXcanSAS standard. also ran ruff which cleaned up the rpm in units and accessors and si.
codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@pkienzle
Copy link

Are these full 3D volumes? Or are they 2D detector surfaces in a 3D volume?

Regardless, please add issues to the sasmodels project saying what updates are needed to resolution2D.

@pmneves7
Copy link
Author

Are these full 3D volumes? Or are they 2D detector surfaces in a 3D volume?

Regardless, please add issues to the sasmodels project saying what updates are needed to resolution2D.

Currently I am just generalizing the dataset object to also support holding the Qz of each detector pixel (or whatever the equivalent in TOF data would be, which does measure a 3D volume). So for a CW SANS instrument this will be 2D detector surfaces with 3D coordinates.

Ultimately, we will be able to combine many 2D detector surfaces (or TOF 3D volumes) into one 3D volume using the NDrebinner on pull request #178 also optionally taking into account the transformations from the lab reference frame to the sample reference frame.

We will want to generalize the resolution function to 3D for this as well. Pederson and Harris originally developed this:
https://journals.iucr.org/paper?S0021889890003946
https://journals.iucr.org/paper?S0021889894010617
And I took a stab at it as well:
Appendix B: https://journals.iucr.org/j/issues/2024/06/00/jl5095/index.html
And coded it in Matlab:
https://github.com/paulneves77/GRIP/blob/main/GRIP/callbacks/GRIP_res_callbacks.m

Note that the ideal way to model 3D data is to fit directly to measured datapoints in 3D where the model is convolved with a 3D resolution function approximation. For visualization both can be rebinned in the same way for plotting in 1D, 2D, or 3D.

Copy link
Collaborator

@krzywon krzywon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have work from two other branches included in it, which can make resolving review comments difficult. You might want to set the base branch for this PR to the NDrebin branch to make this a bit easier, not only for resolving differences, but also rebasing, and reviewing purposes.

Overall, this looks pretty good, but see my comments.

@pmneves7
Copy link
Author

pmneves7 commented Nov 17, 2025

This seems to have work from two other branches included in it, which can make resolving review comments difficult. You might want to set the base branch for this PR to the NDrebin branch to make this a bit easier, not only for resolving differences, but also rebasing, and reviewing purposes.

Overall, this looks pretty good, but see my comments.

I tried to make the NDrebin branch very separate. I don't think they conflict? Maybe I am missing something? I admit that the rpm changes are confusing, but I don't know how to roll back just the changes to the quantities directory in my branch.

edit: I see, I left NDrebin in this branch. That doesn't need to be in this branch. I will remove it from this branch.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

Copy link
Collaborator

@krzywon krzywon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more suggestions that should help with the code scene analysis. Otherwise, this is getting close.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

…examplefile.h5. Also added support for when the NX_class attribute has a canSAS_class name. Also fixed bug when no instrument, sample etc are given in the file.
codescene-delta-analysis[bot]

This comment was marked as outdated.

…multisasentry_multisasdata.h5, nxcansas_1Dand2D_multisasdata.h5, and nxcansas_1Dand2D_multisasentry.h5 test data files
codescene-delta-analysis[bot]

This comment was marked as outdated.

@pmneves7
Copy link
Author

pmneves7 commented Dec 2, 2025

Fixed the unit tests. Details of fixes below:

test/sasdataloader/utest_sasdataload.py:67: AttributeError: I got rid of the "raw" part of the metadata, so I added that back in.

test/sasdataloader/utest_sasdataload.py:375: KeyError:

  • The test h5 file (sasdata/test/sasdataloader/data/simpleexamplefile.h5) has the NX_class node attributes set to canSAS_class values like SASentry. This is technically not conforming to the canSAS standard. However, we can accomodate it by allowing canSAS_class names to be in NX_class attributes.
  • Another issue was that SASentry was a byte string.

test/sasdataloader/utest_sasdataload.py:414: AssertionError :
The sascollimation groups in the test h5 file were missing the canSAS_class attribute in sasdata/test/sasdataloader/data/nxcansas_1Dand2D_multisasentry.h5. This is a required attribute for the canSAS h5 file. I have added this to the h5 file. This was also an issue in nxcansas_1Dand2D_multisasentry_multisasdata.h5 and nxcansas_1Dand2D_multisasdata.h5. NOTE that this may be an issue with the mantid cansas generation.

Copy link
Contributor

@DrPaulSharp DrPaulSharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments from me here.

@krzywon
Copy link
Collaborator

krzywon commented Dec 3, 2025

I think once the comments by @DrPaulSharp are fixed, this might be ready to merge.

codescene-delta-analysis[bot]

This comment was marked as outdated.

…ged name of parse_float_first to parse_float
codescene-delta-analysis[bot]

This comment was marked as outdated.

…ta and nxcansas_1Dand2D_multisasentry where the detector distance was read wrong because of the difference between node.astype(float)[0] and float(node[0].astype(str))
Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates Passed
3 Quality Gates Passed

See analysis details in CodeScene

Quality Gate Profile: The Bare Minimum
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.

Copy link
Collaborator

@krzywon krzywon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready.

@pmneves7 pmneves7 merged commit 03da937 into refactor_24 Dec 4, 2025
11 checks passed
@pmneves7 pmneves7 deleted the refactor_24_3D branch December 4, 2025 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants