scipp / scippnexus

h5py-like utility for NeXus files with seamless scipp integration
https://scipp.github.io/scippnexus/
BSD 3-Clause "New" or "Revised" License
3 stars 3 forks source link

Support loading NXdetector with NXevent_data that lacks `event_time_zero` #201

Closed SimonHeybrock closed 3 months ago

SimonHeybrock commented 3 months ago

I think this should be possible. It might just be the mapping to pixels that assumes it exists. From what I can tell, this requires relatively simple changes in:

  1. NXevent_data (many places, but all simple, I think).
  2. The grouping function that NXdetector calls if it contains an NXevent_data at https://github.com/scipp/scippnexus/blob/6e4e566c56f6375312fa2d3aa03c4c63ee30687c/src/scippnexus/nxdata.py#L768-L770 (simply add a branch, checking if event_time_zero coord exists).

Make sure to add tests for both cases:

jokasimr commented 3 months ago

After looking a little bit closer at this it seems like the NXevent_data group that causes trouble in the Amor file has neither event_time_offset, event_id, event_time_zero or event_index.

But it has several of the fields that a NXlog should have, for example time, value, maximum_value, minimum_value, and average_value. So my guess is that this data group just doesn't have the right NX_class attribute, it should have been an NXlog.

If that's the case I think it makes little sense for us to try to accommodate this deviation in the nexus reader, and we should probably close this issue.

I'll try to manually change the attribute to be NXlog and reload the file in the workflow.

SimonHeybrock commented 3 months ago

Is the NXevent_data nested inside an NXdetector? If so, this is an interesting case that I need to consider.

That aside, I think this issue can be kept open, since having no event_time_zero does seem valid.

jokasimr commented 3 months ago

Yes it's nested inside a NXdetector, I'll send you the file.