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

Big rewrite #117

Closed SimonHeybrock closed 1 year ago

SimonHeybrock commented 1 year ago

This adds, as scippnexus.v2, the future version of ScippNexus. The trigger for this rewrite were significant performance issues with small files that contain thousands of groups and datasets. However, it also attempts to resolve some complicated logic of the old implementation, which did lead, e.g., to hard to understand stack traces in exceptions.

To use this, just try import scippnexus.v2 as snx. The basic API is the same as before.

Apart from internal design and significant performance improvements (not affecting large datasets), there are a number of changes:

For review:

Bugfixes:

SimonHeybrock commented 1 year ago

Apart from addressing your comments, I pushed another small change the switches out the cached_property for manual lazy init for the Group._children and Group._nexus properties. This should make it easier to see through the logic. I kept cached_property for the other methods/properties that "just" return something simple that is computed.

jl-wynen commented 1 year ago

I honestly don't see the benefit. Was the logic unclear before?

SimonHeybrock commented 1 year ago

I honestly don't see the benefit. Was the logic unclear before?

In particular the _children felt odd: We write to the return value, but it is not simply returning an actual field of the class instance. Rather, it is returning something that it constructed in the property. We basically relied on cached_property to do this exactly once, and this may be unexpected to the reader.

SimonHeybrock commented 1 year ago

Should be ready for another review @jl-wynen.

SimonHeybrock commented 1 year ago

Tracking next steps in #120.

SimonHeybrock commented 1 year ago

@jl-wynen I fixed another small issue, and pushed some extra horrible code to deal with event-data fields embedded in NXdetector, to somewhat restore the ability to load SNS files. I am not planning further changes in this PR, so this is ready for a "final" look.