Skip to content

Add bit-depth-image#1262

Closed
graeme-winter wants to merge 1 commit into
nexusformat:mainfrom
graeme-winter:bit-depth-image
Closed

Add bit-depth-image#1262
graeme-winter wants to merge 1 commit into
nexusformat:mainfrom
graeme-winter:bit-depth-image

Conversation

@graeme-winter

Copy link
Copy Markdown
Contributor

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

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

Copy link
Copy Markdown
Contributor

It could also be considered to extend NXdetector, too.

@phyy-nx

phyy-nx commented May 24, 2023

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown

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

phyy-nx commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

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 phyy-nx self-assigned this Jun 15, 2023
@phyy-nx phyy-nx mentioned this pull request Jun 19, 2023
@phyy-nx

phyy-nx commented Jun 19, 2023

Copy link
Copy Markdown
Contributor

Changes made in new PR #1284

@phyy-nx phyy-nx closed this Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

NXmx should (or should not?) mention bit_depth_image

4 participants