Better rewrite of NXdata scaling_factor and offset fields #1343
Conversation
Change NXdata scaling_factor to refer to "plotted" data Change NXdata to refer to "corrected" data, in addition to "physical" data, since it describes units of photons
This resolves ambiguity if there is more than one signal For NXmx specify data_scaling_factor and data_offset since the field data is named in the NXdata group
- NXmx: fully define pedestal, bias, and gain and how they relate to offset and scaling_factor - Add reserved suffixes _scaling_factor and _offset - NXdata: define better how the stored values in FIELDNAME are converted to physical values - NXdata: more clarification on how scaling_factor and offset without FIELDNAME are ambiguous
|
691f56a addresses comments from the review of #1333:
Thanks for the feedback! This is ready for further review :) |
|
@paulmillar @PeterC-DLS I've come back to this after a while and responded to your comments. Can you review and resolve the comments that are ok now? Also, I'm not seeing why the checks are failing. Any help on this @PeterC-DLS? Thanks!! |
|
Feedback from Telco: need to deprecate NXdata's scaling_factor and offset instead of replacing them with FIELDNAME_scaling_factor and FIELDNAME_offset |
|
Woops I actually had deprecated the original fields. So @paulmillar and @PeterC-DLS this is ready to review. Thanks! |
|
Hi @phyy-nx
My earlier comment has been addressed. I've added a NON-BLOCKING comment, meant as a general observation rather than a comment about this particular pull request. The pull request looks reasonable to me. |
|
@paulmillar Once this PR is resolved, visibility of your non-blocking comment will drop. I suggest elevating that visibility by making your comment a new issue (use the GitHub feature). |
|
Hi @prjemian
Thanks. I've followed your suggestion and created #1400. |
- NX_FLOAT -> NX_NUMBER for scaling_factor and offset - Sync language with number #1396 for the FIELDNAME convention
|
I think I've addressed all the comments @paulmillar @PeterC-DLS. If folks agree, can you resolve your comments and approve it? Approving here doesn't mean merging as it likely needs a vote. Just that it's ready for a vote. |
|
This PR is ready for a NIAC vote. Please vote with an emoji on this comment. Options include thumbs up for yes, thumbs down for no, and anything else to abstain. Thank you. Voting will close in two weeks. |
|
Hello all, the vote has passed with 15 in favor. Thanks for the reviews and attention. |
New parameters as of nexusformat/definitions#1343
New parameter as of nexusformat/definitions#1343. This is not full support of the parameter or of data_offset, but it's one of the use cases needed, namely reading a single gain value
Adds
FIELDNAME_scaling_factorandFIELDNAME_offsetas fields to NXdata. These fields should be used instead ofscaling_factorandoffset, as it is ambiguous which fields to apply them to in the case of multiple signals._scaling_factorand_offsetare added as reserved suffixes.Additionally adds a formula for how to apply these fields:
For NXmx, adds clarification these fields can be used as pedestal and gain correction fields, and defines these terms. Also the possible rank options are elaborated. These rank options were implied (in my opinion) in the original wording, but in NXmx I made it more explicit.
Closes #1332. Same set of changes as in #1333 with an additional commit to resolve feedback from #1333.