Big rewrite #117
Merged
Big rewrite #117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add error handling Fix reading of fields Add NXobject subclass handling Refactor for more automatic child handling Introduce multiple map layers, I think this does not work Refactor to new approach Customize field dims via strategy Refactor Rename Basic NXdata support Cleanup Absorb errors handling into Field Cleanup Allow for dtype override Pass (base) definitions from parents Translate sel for children Bin-edge handling Undo change to unrelated file
|
Should be ready for another review @jl-wynen. |
jl-wynen
reviewed
Apr 3, 2023
This reverts commit 77e2d7c.
10 tasks
|
Tracking next steps in #120. |
|
@jl-wynen I fixed another small issue, and pushed some extra horrible code to deal with event-data fields embedded in |
jl-wynen
approved these changes
Apr 5, 2023
SimonHeybrock
added a commit
that referenced
this pull request
Nov 2, 2023
I believe this was added in #117 when considering to not group by pixel during loading. In the end that was not the chosen solution, but apparently I forgot to remove this now unused helper.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
DataGroupis not better at preserving "partial" information, if assembly fails.NX*geometryclasses are loaded asDataGroupwithout processing. We will provide post-processing functions for grouping this bydetector_number, etc.NXevent_datais not automatically grouped bydetector_number. There is a post-processing function for this, but we will need to add some that operate recursively.For review:
base.py.class Fileand its helper functions are mostly (but not completely) unchanged.nxdata.py.nxtransformations.py, which I plan to refactor in a follow-up into a couple of user-facing functions for resolving and executingdepends_on-chains.Bugfixes:
shape=(0,)are now correctly loaded as datetime if they should. Previously this always returned a time (offset).shape=(0,)are now correctly loaded as transformation if they should. Previously this always returned the plain value of the transformation.