Add bit-depth-image - Githubissues

nexusformat / definitions

Definitions of the NeXus Standard File Structure and Contents

https://manual.nexusformat.org/

Other

26 stars 55 forks source link

Add bit-depth-image #1262

Closed graeme-winter closed 1 year ago

graeme-winter commented 1 year ago

Fixes #1258 by including it, using effectively the same definition as is currently in the field from DECTRIS

sanbrock commented 1 year ago

It could also be considered to extend NXdetector, too.

phyy-nx commented 1 year ago

Feedback from the Telco:

Should we also add bit sign, fast vs. slow, endian, etc.? We think that discussion could be had outside of this issue though.
Should this be in NXmx only as @sanbrock or should it be in NXdetector?
Is NXdetector even the right place? Should it be in NXdata instead?
Regardless, this will need a NIAC vote, which will be in the June code camp.

graeme-winter commented 1 year ago

It is in use now in the field and was a specific proposal - fast vs. slow are encoded in the existing standard, endian is (I think?) already handled elsewhere, and the dtype is (I think) explicitly encoded in HDF5 so we don't need to have the signed-ness really?

Anyway, I look forward to the outcome of the vote on adopting a tag which is already in use.

ndevenish commented 1 year ago

Should we also add bit sign, fast vs. slow, endian, etc.? We think that discussion could be had outside of this issue though.

Certainly signedness has caused confusions in dealing with this issue, as when trying to handle these mismatches we've had to make educated guesses as to whether we need to convert or truncate when converting to/from the declared bit depth. Endian is, I suppose, strictly missing, but I don't think we've seen any live examples of this being important.

But yes, somewhat orthogonal to the issue of this specific field.

phyy-nx commented 1 year ago

Discussion from Code Camp:

I showed DIALS code that uses bit_depth_image to handle safe/unsafe integer conversions between NumPy and "flex", which is the internal DIALS/cctbx memory management implementation.
Unofficial straw poll in the Code Camp indicated we were in favor of moving this to NXdetector as well
Update data type from NX_INT to NX_POSINT (no change to Dectris or DIALS)
Modify language in NXdetector bit_depth_readout: change "must not" to "might not"

I'll get these changes ready. Thanks again to @graeme-winter.

phyy-nx commented 1 year ago

Changes made in new PR #1284