Skip to content

Replacement Apply and Resample #5436

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.

Sign up for GitHub

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

Jump to bottom
Merged
wyli merged 21 commits into from
Nov 25, 2022
Merged

Replacement Apply and Resample #5436

wyli merged 21 commits into from
Nov 25, 2022

Conversation

@atbenmurray
Copy link
Contributor

@atbenmurray atbenmurray commented Oct 30, 2022 •
edited by wyli
Loading

Signed-off-by: Ben Murray [email protected]

Description

This is part of the work towards #4855. It adds:

  • a lazy apply method
  • A transform-like wrapper for apply called Apply
    - MetaMatrix and related functionality to represent abstracted grid and matrix transforms with metadata
  • A universal resample function that can be used to apply grid / matrix transforms
    - Functional spatial and croppad implementations that define but don't apply transforms

Types of changes

  • 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).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@atbenmurray atbenmurray mentioned this pull request Oct 30, 2022
Closed
7 tasks
@atbenmurray atbenmurray mentioned this pull request Oct 30, 2022
Open
33 tasks
wyli
wyli reviewed Nov 1, 2022
View reviewed changes
@wyli
Copy link
Contributor

wyli commented Nov 1, 2022 •
edited
Loading

/black
thanks, I think this PR introduces quite a few APIs at a time and some of them (MatrixFactory/MetaMatrix) need more discussions... is it possible to make it smaller, for example, only including croppad, apply, and an example of SpatialCrop transform, so that we can create a working example of lazy cropping, then finalise the functional and apply API? cc @ericspod @rijobro @Nic-Ma

@atbenmurray
Copy link
Contributor Author

atbenmurray commented Nov 1, 2022 •
edited
Loading

thanks, I think this PR introduces quite a few APIs at a time and some of them (MatrixFactory/MetaMatrix) need more discussions... is it possible to make it smaller, for example, only including croppad, apply, and an example of SpatialCrop transform, so that we can create a working example of lazy cropping, then finalise the functional and apply API? cc @ericspod @rijobro @Nic-Ma

The problem is that MatrixFactory/MetaMatrix are the classes by which a lot of the internal stuff for lazy resampling is done. They could certainly change before the PR goes in, but that would necessitate some plumbing to happen and I don't know if I'll have time before I am on holiday. The key thing here at the moment is that this PR doesn't change any existing behaviour so if we want to refactor we certainly can.

What they do:

  • MatrixFactory is an attempt to wrap up the various affine operations in a way that it can be simply constructed by either defining dimensions, device etc or by passing a tensor so that is determined automatically, and used in an OO way.
  • MetaTensor is the transformation + metadata that accompanies it. It contains either a Matrix or a Grid object, depending

In theory, one or both could go, but it might make everything somewhat verbose. Neither are supposed to be user-facing

Might be a good topic for tomorrow, but I am trying to get the PRs as populated as possible for tonight.

@atbenmurray atbenmurray self-assigned this Nov 2, 2022
atbenmurray added a commit that referenced this pull request Nov 2, 2022
@atbenmurray
Adding in placeholders and functions required for functional, array a…
c0c5558 
…nd dictionary transforms to operate while waiting for PR #5436

Signed-off-by: Ben Murray <[email protected]>
atbenmurray added a commit that referenced this pull request Nov 2, 2022
@atbenmurray
Adding in additional placeholders and functions required for function…
7a41b3f 
…al, array and dictionary transforms to operate while waiting for PR #5436

Signed-off-by: Ben Murray <[email protected]>
atbenmurray added a commit that referenced this pull request Nov 2, 2022
@atbenmurray
Adding in additional placeholders and functions required for function…
57c66e4 
…al, array and dictionary transforms to operate while waiting for PR #5436

Signed-off-by: Ben Murray <[email protected]>
atbenmurray added a commit that referenced this pull request Nov 2, 2022
@atbenmurray
Putting comments on utils methods that have been duplicated from PR #…
ae486d1 
…5436

Signed-off-by: Ben Murray <[email protected]>
wyli
wyli reviewed Nov 4, 2022
View reviewed changes
@wyli wyli force-pushed the lr_apply_2 branch 3 times, most recently from 95ed4a1 to 87c4356 Compare November 12, 2022 23:03
atbenmurray and others added 10 commits November 23, 2022 09:51
@atbenmurray @wyli
Commit of functionality on replacement lr_apply branch due to issues …
d557549 
…with rabasing and automatic signatures

Signed-off-by: Ben Murray <[email protected]>
@atbenmurray @wyli
adding functional tests for flip, resize and spacing
49e7ed6 
Signed-off-by: Ben Murray <[email protected]>
@pre-commit-ci @wyli
[pre-commit.ci] auto fixes from pre-commit.com hooks
61dbe11 
for more information, see https://pre-commit.ci
@atbenmurray @wyli
Adding additional spatial functional unit tests
9c11d4d 
Signed-off-by: Ben Murray <[email protected]>
@atbenmurray @wyli
Work on apply and matmul tests
77d7c31 
Signed-off-by: Ben Murray <[email protected]>
@pre-commit-ci @wyli
[pre-commit.ci] auto fixes from pre-commit.com hooks
828a7d8 
for more information, see https://pre-commit.ci
@atbenmurray @wyli
Adding tests for matmul and matrix_matrix
23a05fa 
Signed-off-by: Ben Murray <[email protected]>
@atbenmurray @wyli
Removing spatial functional transforms from this PR
a67ef17 
Signed-off-by: Ben Murray <[email protected]>
@atbenmurray @wyli
Removing croppad functional transforms from this PR
b25a6cb 
Signed-off-by: Ben Murray <[email protected]>
@atbenmurray @wyli
Removing spatial functional tests as they are no longer part of this PR
42f33a2 
Signed-off-by: Ben Murray <[email protected]>
monai-bot and others added 5 commits November 23, 2022 09:51
@monai-bot @wyli
[MONAI] code formatting
1177263 
Signed-off-by: monai-bot <[email protected]>
@wyli
minor updates
1492f40 
  - `_pending_operations` defaults to `MetaObj.get_default_applied_operations()`
  - removes the unused `Apply` class
  - fixes unit tests, style checks
  - torch.pi doesn't exist before torch 1.10

Signed-off-by: Wenqi Li <[email protected]>
@wyli
apply/matmul/resample MVP
2d4122b 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
rearrange modules, adds back matrix/grid types
6c4db35 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
matmul -> combine_transforms
9eb5279 
Signed-off-by: Wenqi Li <[email protected]>
wyli added 3 commits November 23, 2022 11:10
@wyli
remove unused
17574cb 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
adds docstrings
e067b6e 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
adds testing
c5aa26c 
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli marked this pull request as ready for review November 23, 2022 13:10
wyli
wyli reviewed Nov 23, 2022
View reviewed changes
wyli
wyli approved these changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atbenmurray please make more changes if needed and let me know when you think it's ready.. cc @Nic-Ma this mainly introduces necessary APIs monai.transforms.lazy.*

@wyli wyli mentioned this pull request Nov 23, 2022
Open
@wyli
Copy link
Contributor

wyli commented Nov 24, 2022

/build

@wyli wyli merged commit 2c7ba9e into dev Nov 25, 2022
@wyli wyli deleted the lr_apply_2 branch November 25, 2022 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli approved these changes

@Nic-Ma Nic-Ma Awaiting requested review from Nic-Ma Nic-Ma is a code owner

@rijobro rijobro Awaiting requested review from rijobro

@ericspod ericspod Awaiting requested review from ericspod ericspod is a code owner

Assignees

@atbenmurray atbenmurray

Labels

None yet

Projects

No open projects
MONAI v1.1
Status: Done

Milestone

Lazy resampling (+ cache-based datase...

Development

Successfully merging this pull request may close these issues.

4 participants

@atbenmurray @wyli @monai-bot