Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stream2 Start Message Fields #6

Open
GDYendell opened this issue Aug 3, 2023 · 10 comments
Open

Stream2 Start Message Fields #6

GDYendell opened this issue Aug 3, 2023 · 10 comments

Comments

@GDYendell
Copy link

We have now begun testing stream2 on I03 at DLS and have found there are some fields missing that are relied upon. We would like to discuss adding the following legacy fields to stream2:

  • images_per_trigger
  • number_of_triggers
  • roi_mode
  • bit_depth_image
  • bit_depth_readout

I think bit_depth_image is the only absolutely critical one for our automated processing. But the others are used to do some sanity checking that the acquisitions are working as expected.

Related to #2.

cc @graeme-winter

@graeme-winter
Copy link

@diego-gaemperle any idea on when you may get a chance to look at this?

@diego-gaemperle
Copy link

@GDYendell @graeme-winter I apologize for the late answer. I didn't see the issues since I've messed up my notifications. For stream2 we took the decision to kick out everything that is irrelevant for the data analysis in the start message.
We will discuss this internally and get back to you with either more questions or a specific path forward.

In general there will be a new release probably soon before end of the year. We can also supply a test (development) version before the release date for you to test with if this would help.

@kalcutter
Copy link
Contributor

@GDYendell We are conservative adding new fields since fields generally can't be removed once they have been introduced. Our general requirements for new stream2 fields are (1) they should directly describe the series / data and (2) have a clear and relevant use-case. In this light, we are open to adding roi_mode and some field to describe the image data type. We are against adding the other fields at this time. Instead of bit_depth_image, we would like to add a field image_dtype that describes the pixel data type of the image. This has the advantage of being able to describe signed/float pixel data. In particular, we propose adding the following fields:

  • roi_mode - [TextString, Optional] String describing the ROI mode. If no ROI mode is set, the field is not present.
  • image_dtype - [TextString] String identifying the pixel data type. Currently, either: "uint8", "uint16", or "uint32".

Please let us know if this proposal would meet your requirements.

@kalcutter
Copy link
Contributor

After further reflection, I think it would be better to use an integer instead of a string for image_dtype. In particular, I would use the integer of the tag number used for the typed array of the image data. This way, similar logic can be used for both (e.g. enum stream2_typed_array_tag could be used).

@kalcutter
Copy link
Contributor

Could you also provide more information about the "roi_mode" use-case? I am wondering the best way to convey the ROI information. For example, it might be better to add fields like roi_offset_x and roi_offset_y instead of the API/detector-specific "roi_mode" string value.

@kalcutter
Copy link
Contributor

I have updated the documentation and examples for image_dtype. Please take a look at https://github.com/dectris/documentation/tree/wip-image_dtype/stream_v2 and give any feedback.

@GDYendell
Copy link
Author

I think this looks good. I assume you approve @graeme-winter?

@kalcutter
Copy link
Contributor

FYI: For the signed data "int8", "int16" and "int32", should be used, respectively. This is not in our documentation since we only produce unsigned data currently.

@kalcutter
Copy link
Contributor

image_dtype will be available in our next software release.

@graeme-winter
Copy link

I think this looks good. I assume you approve @graeme-winter?

Indeed I do, because this was +/- exactly what I asked for 😉

Thank you

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

No branches or pull requests

4 participants