DiamondLightSource / nexgen

Next Generation Nexus Generator
BSD 3-Clause "New" or "Revised" License
2 stars 9 forks source link

Fix missing parts of nexus file #219

Closed DominicOram closed 6 months ago

DominicOram commented 6 months ago

A user has noticed that the Hyperion nexus file does not have all the fields/attributes that a GDA file had:

we've got a DLS dataset from detector E-32-0117 which we /think/ is
the one on I03. That is very recent data and we noticed that some
items are not provided in the *_master.h5 file. They seem to be
present in the *_meta.h5 file, but should probably be linked to from
the master.h5 file like some others.
The items are:
 /entry/instrument/detector/detectorSpecific/data_collection_date  -> /_dectris/data_collection_date
 /entry/instrument/detector/detector_number                        -> /_dectris/detector_number
 /entry/instrument/detector/detectorSpecific/eiger_fw_version      -> /_dectris/eiger_fw_version
 /entry/instrument/detector/detectorSpecific/ntrigger              -> /_dectris/ntrigger
The most crucial one is the detector_number (or serial_number) that
gives us the detector serial number - and together with the data
collection date (which we take from /entry/start_time instead of the
above) it uniquely defines any special settings required for a
specific beamline configuration.

PS: while looking at those HDF5 files it might also be a good idea to

reinstate the "units" attribute on
       /entry/instrument/detector/detector_readout_time
       /entry/instrument/detector/threshold_energy

add the "units" attribute to
      /entry/instrument/detector/count_time

Acceptance Criteria

graeme-winter commented 6 months ago

Thank you: once I have a set of 3rd party tests to execute I will add them to this ticket also

dperl-dls commented 6 months ago

Moved this issue here because it's really a Nexgen issue - nothing should need to change in Hyperion to make this work, and the test might as well live here too

dperl-dls commented 6 months ago

@graeme-winter @DominicOram How urgently do we want to fix this immediate issue compared with doing everything? i.e. deploy the changes described above and then implement any other missing data the tests expose, or vice versa?

DominicOram commented 6 months ago

I think we can do it in two PRs, one to fix the immediate missing parts and a second to do the wider test. I don't think we should break it into 2 issues though as I'd worry we'd end up doing part 1 and never getting round to part 2. Given it's not been noticed until now I don't think we need to rush to do the first part, we would probably end up naturally getting it merged/released in a week or two anyway.

dperl-dls commented 6 months ago

detector_number should have already been in there but it is specified in some eiger mapping as serial_number - which seems to be the used form for i24 - so do we want both of these?

DominicOram commented 6 months ago

According to https://manual.nexusformat.org/classes/base_classes/NXdetector.html#nxdetector detector_number and serial_number are both in the there but different things:

Not sure which we are talking about here

dperl-dls commented 6 months ago

it sounds like they (GP) are using detector_number to refer to the serial number; in our meta files likewise _dectris/detector_number contains the serial number as a string... so do we replicate GDA behaviour or conform to spec?

DominicOram commented 6 months ago

I defer to @graeme-winter but I would prefer conform to spec.

graeme-winter commented 6 months ago

@graeme-winter @DominicOram How urgently do we want to fix this immediate issue compared with doing everything? i.e. deploy the changes described above and then implement any other missing data the tests expose, or vice versa?

I would recommend:

Feel reasonable?

graeme-winter commented 6 months ago

it sounds like they (GP) are using detector_number to refer to the serial number; in our meta files likewise _dectris/detector_number contains the serial number as a string... so do we replicate GDA behaviour or conform to spec?

This should be serial number

The spec detector number is the use case where you have multiple similar detectors, here the request is for "detector number" i.e. the serial number of the detector, IIRC, which is what we link for GDA

dperl-dls commented 6 months ago

Yes, it's what we link in GDA, but it's what nexgen otherwise (correctly, according to the doc linked above) calls serial_number

graeme-winter commented 6 months ago

Yes, it's what we link in GDA, but it's what nexgen otherwise (correctly, according to the doc linked above) calls serial_number

If that is correct according to the NXmx specification, then it is correct and we can assert it as such

I don't have a canonical DECTRIS file-writer version to hand to check... but if the standard says "do this" we can refer compaints there 👍

dperl-dls commented 6 months ago

Can we spin the "test comparing nexgen to legacy nexus files" into a new issue and close this one with the immediate changes made?

DominicOram commented 6 months ago

Sure, but I still think it's quite high priority so can you add the new issue near the top of the sprint?