scipp / ess

European Spallation Source facility bespoke, neutron scattering tools based on scipp.
https://scipp.github.io/ess/
BSD 3-Clause "New" or "Revised" License
3 stars 3 forks source link

Add `nexus` submodule with components for building customized NeXus file loaders #139

Closed SimonHeybrock closed 1 year ago

SimonHeybrock commented 2 years ago

For my own understanding: Is the reason for implementing this interface that it makes assumptions about the structure of a file, e.g. an instrument knows that it contains fields and detectors as direct children? Is this why this code is in ess and not scippnexus?

I think we may eventually decide to move this into scippnexus. I did it here initially, since I think we will likely need some iterations, and want to try how it integrates with implementation of data reduction workflows. Having it in scippnexus would tie us more to releases and complicate this.

jl-wynen commented 2 years ago

But then I am unclear about what benefit this has over group[NXInstrument].

SimonHeybrock commented 2 years ago

But then I am unclear about what benefit this has over group[NXInstrument].

You mean the additions in this PR? It avoids a significant amount of boilerplate code, I think.

jl-wynen commented 2 years ago

But then I am unclear about what benefit this has over group[NXInstrument].

You mean the additions in this PR? It avoids a significant amount of boilerplate code, I think.

But why not use group[NXInstrument][()] instead of Instrument.from_nexus(group)?

SimonHeybrock commented 2 years ago

But then I am unclear about what benefit this has over group[NXInstrument].

You mean the additions in this PR? It avoids a significant amount of boilerplate code, I think.

But why not use group[NXInstrument][()] instead of Instrument.from_nexus(group)?

The former would necessarily load everything, whereas the approach implemented here allows for customization?

jl-wynen commented 2 years ago

The former would necessarily load everything, whereas the approach implemented here allows for customization?

Do you mean because the dataclasses here do not have to include all fields in a nexus group? If so, wouldn't this be specific to a use case?

SimonHeybrock commented 2 years ago

The former would necessarily load everything, whereas the approach implemented here allows for customization?

Do you mean because the dataclasses here do not have to include all fields in a nexus group? If so, wouldn't this be specific to a use case?

Yes, the idea is that each instrument would customize this, e.g., to load (ore ignore) specific instrument components.

SimonHeybrock commented 2 years ago

I finally addressed some of the comments here. See answers and updates. As mentioned face-to-face, I am certain that this will require iterations. For now, we can avoid documenting and advertising it widely such that we can try it in practice for a small subset of our applications.

SimonHeybrock commented 1 year ago

I cannot reproduce this error locally. Tried both the latest scippnexus and 0.3.2. Can one of you reproduce this?

jl-wynen commented 1 year ago

No, the tests pass on my workstation

SimonHeybrock commented 1 year ago

The CI fail comes from a NumPy Deprecation warning, triggered by and old h5py (h5py=3.1.0). This was fixed one and a half years ago in h5py=3.2.0. Can we update @nvaytet, or is something (Mantid?) holding us back?

nvaytet commented 1 year ago

Yes, Mantid is forcing us to pin I think, although I have not tried in a while, but hdf5=1.10 is still required by the mantid conda recipe. See the discussion in Slack: Screenshot at 2022-11-14 12-23-54

SimonHeybrock commented 1 year ago

So.... should we suppress the warning instead? Or can we avoid the Mantid dependency in the CI?

nvaytet commented 1 year ago

Not sure, I've just asked on the Mantid slack if we could move hdf5 to a more recent version. We'll see what happens.

SimonHeybrock commented 1 year ago

I don't think a change there would happen on a timescale that works for us. I think we should begin considering dropping Mantid from the CI, or finding ways to split things.