Repository navigation
Improve and simplify loading of transformation chains - #243
Conversation
This reverts commit 433e17e.
SimonHeybrock
changed the title
transformations module
e3b4a18 to
048e3ec
Compare
nvaytet
left a comment
nvaytet
left a comment
There was a problem hiding this comment.
I must say I didn't follow all the code changes, but the tests look fine ;-)
| """ | ||
| Represents a depends_on reference in a NeXus file. | ||
|
|
||
| The parent (the full path within the NeXus file) is stored, as the value may be |
There was a problem hiding this comment.
Are parents always guaranteed to be absolute or could they also be relative?
There was a problem hiding this comment.
The class does not check this on init, but in our code it comes from dataset.parent.name, and in h5py name gives the full absolute path.
| value: sc.Variable | sc.DataArray, | ||
| *, | ||
| transformation_type: str, | ||
| select: ScippIndex, |
There was a problem hiding this comment.
Was this select actually never used in the old function?
There was a problem hiding this comment.
Indeed, never used, because it was unclear what to do with potential time selection. Also, since the selection was already applied when loading the underlying group or dataset it made little sense, unless the plan was to apply it to the entire chain, which maybe was the original idea? So maybe it should be considered to forward select when resolving a chain? But I think such a mechanism will be useless 90% of the time, since each chain link can have different length, etc., ...
| raise TransformationError('A transformation needs a vector attribute.') | ||
| return sc.vector(value=self.attrs.get('vector')) | ||
|
|
||
| def __getitem__(self, select: ScippIndex): |
There was a problem hiding this comment.
I am not sure I managed to follow where the select logic was moved. Is is simply that we are not performing any selection on a time-dependent transformation anymore?
There was a problem hiding this comment.
It can, since it is applied when loading the NXlog, before maybe_transformation builds a transformation from it. See also above though.
Co-authored-by: Neil Vaytet <[email protected]>
Co-authored-by: Neil Vaytet <[email protected]>
2 participants
See docstring in
nxtransformations.pyfor an explanation.I have added
class DependsOnwhich seems a much saner way to handle the references, instead of juggling absolute and relative paths. The subclassTransformationChainadditionally hold the chain of transformations starting at the depends_on.Fixes #234 by introducing a new mechanism that can work around the shortcomings of the default data load. See there also for larger context.
Technically this represents a breaking change, since users would have to call
build()after loading groups with transformations. In practice this is however typically not accessed directly — users typically usesnx.compute_positionsinstead of working with the raw chains.