-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Early WIP] - Lazy resampling #4922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…factored apply; initial compose implementation + helpers
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
zoom_factors = [i / j for i, j in zip(src_pixdim_, pixdim_)] | ||
|
||
transform = MatrixFactory.from_tensor(img).scale(zoom_factors) | ||
im_extents = extents_from_shape(img.shape) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we have a chain of lazy transforms, img.shape
will become outdated, correct? because we don't actually run img_t = apply(img_t)
.
so I think accessing img.shape
at this point is inaccurate, this should be something like metadata['spatial_shape']
where metadata
should come from a previous lazy transform or img.shape
if the previous transform evaluates eagerly.
and in this case, the input of this function should include a metadata
or input_shape
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and it seems img
is not really needed in this function (and in the other functionals as well), just need the up-to-date spatial shape information of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'm aware of the issue (chain of lazy transforms). I'm still trying to figure out the best way to handle when / how transforms to shape / extents occur. I'll hopefully have it sorted by the end of the day; just experimenting at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should be able describe the extent change by concatenating the individual extent change operations at the point we perform the stacking of transforms; details to follow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… of CachedTransform
Signed-off-by: Wenqi Li <[email protected]>
Signed-off-by: Wenqi Li <[email protected]>
Signed-off-by: Wenqi Li <[email protected]>
follow-up of #4922 ### Description - minimal interface to track the pending transforms via metatensor - transforms.Flip is modified as an example for discussion - discussion points: - maintaining `pending_operations` and `applied_operations` independently? - the data structure for `pending_operations` element is a python dictionary - transform "functional" refactoring ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [x] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [x] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [x] In-line docstrings updated. - [x] Documentation updated, tested `make html` command in the `docs/` folder. Signed-off-by: Wenqi Li <[email protected]>
follow-up of #4922 ### Description - minimal interface to track the pending transforms via metatensor - transforms.Flip is modified as an example for discussion - discussion points: - maintaining `pending_operations` and `applied_operations` independently? - the data structure for `pending_operations` element is a python dictionary - transform "functional" refactoring ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [x] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [x] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [x] In-line docstrings updated. - [x] Documentation updated, tested `make html` command in the `docs/` folder. Signed-off-by: Wenqi Li <[email protected]> Signed-off-by: KumoLiu <[email protected]>
follow-up of Project-MONAI#4922 ### Description - minimal interface to track the pending transforms via metatensor - transforms.Flip is modified as an example for discussion - discussion points: - maintaining `pending_operations` and `applied_operations` independently? - the data structure for `pending_operations` element is a python dictionary - transform "functional" refactoring ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [x] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [x] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [x] In-line docstrings updated. - [x] Documentation updated, tested `make html` command in the `docs/` folder. Signed-off-by: Wenqi Li <[email protected]> Signed-off-by: Yiheng Wang <[email protected]>
Lazy Resampling - deep refactor (see #4855)
The goal is to provide less memory-intensive, faster and higher quality spatial resampling by adopting the best practice of traditional graphics pipelines.
This change impacts on most of the spatial transform code, and so is also an opportunity to refactor said code to detangle what have become quite complex spatial transforms. While #4911 demonstrates that #4855 is possible, this PR is trying to find a way to decouple elements of this solution from one another and dramatically simplify the task of implementing new transforms in future and by third parties.
Transform refactoring
Due to the complexity of existing transforms and the need to avoid adding more complexity, this refactor looks at the transforms from first principles, attempting to decouple the definition of the transform from its application. This can be seen in the three layers of transform implementation, functional, array and dictionary:
Functional
Functional transforms now have a single responsibility, to generate the definition of the transform.
Array
Array transforms now wrap the Functional transforms, and then decide whether to push the transform to a pending queue or apply it immediately. They do this by descending from LazyTransform, which has a flag
lazy_evaluation
. This could be taken further; the pattern of whether to push to pending or apply immediately could be part of LazyTransform functionality.Dictionary
Dictionary transforms wrap Array transforms, applying them once for each key. There is potential here to further reduce the code by having a standard 'apply to each specified key' function that can be called by subclasses.
The amount of code is dramatically reduced as a result, at least in the transforms reimplemented so far.
Compose compilers
As part of looking at this refactor, it is becoming obvious that there are a few places where the list of transforms to be executed are manipulated as they are being executed. Each place in the code that does this does so with the assumption that no-one else is doing it. As such, one compose manipulator will "win" and the others do not appear to be applied at all.
One way around this is to specify "compose compilers". These are functions that will take an input list of transforms and compile it to a new list of transforms. The benefit of compose compilers is twofold:
Types of changes
./runtests.sh -f -u --net --coverage
../runtests.sh --quick --unittests --disttests
.make html
command in thedocs/
folder.TODO:
Spatial transform implementation
See Table: Implementation by Spatial Transform for detailed progress:
LazyTransform and RandomTransform interfaces for 3rd party use
Interfaces that indicate capability without needing to adopt implementations:
Compose compilation
This is done if we go with compose compilers as a unified mechanism for lazy resampling, smart datasets, etc.:
MetaTensor updates
Optionally make MetaTensors able to share read only access to underlying Tensor for multi-sampling transforms:
Lazy resampling compose
This is done if we go with augmenting compose and leaving smart datasets alone:
Table: Implementation by spatial transform
Table: Implementation by Crop / Pad transform