Skip to content
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

112 combining spatial transforms #131

Merged
merged 13 commits into from
Mar 9, 2020
Merged

112 combining spatial transforms #131

merged 13 commits into from
Mar 9, 2020

Conversation

wyli
Copy link
Contributor

@wyli wyli commented Mar 2, 2020 •
edited
Loading

Fixes #111 fixes #112.

Description

implemented in pytorch for the combined spatial transforms:

  • affine transforms
  • randomised version of the transforms
  • random elastic deformations
  • unit tests
  • pipeline speed test (GPU/CPU)

This PR includes a demo and a speed test.
Observations:

  • the combined transforms are faster than the individual ones on both CPU/GPU
  • can have a good preprocessing speed provided the output stays on GPU -- if these transforms are followed by some CPU operations, there're significant data transfer costs.
  • need to run torch.multiprocessing.set_start_method('spawn') beforehand if both transforms and networks request GPU processing
  • 3D elastic deformation field is approximated by gaussian smoothing over the coordinates because 3D cubic spline upsampling is not supported by pytorch

Status

Ready

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New tests added to cover the changes
  • Docstrings/Documentation updated

@wyli wyli changed the title WIP combining spatial transforms WIP 112 - combining spatial transforms Mar 2, 2020
ericspod
ericspod reviewed Mar 2, 2020
View reviewed changes
Copy link
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

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

We do need tests for this transform and example of use. We could also add transforms for mirroring in XYZ dimensions and transposing two dimensions.

monai/transforms/transforms.py Show resolved Hide resolved
monai/transforms/utils.py Outdated Show resolved Hide resolved
monai/transforms/transforms.py Show resolved Hide resolved
@wyli wyli changed the title WIP 112 - combining spatial transforms combining spatial transforms Mar 5, 2020
@wyli wyli changed the title combining spatial transforms 112 combining spatial transforms Mar 5, 2020
@wyli wyli requested a review from Nic-Ma March 5, 2020 21:57
@wyli
Copy link
Contributor Author

wyli commented Mar 5, 2020 •
edited
Loading

this is ready from my side, could you review? @ericspod @atbenmurray @Nic-Ma

Nic-Ma
Nic-Ma reviewed Mar 8, 2020
View reviewed changes
monai/transforms/utils.py Outdated Show resolved Hide resolved
Nic-Ma
Nic-Ma reviewed Mar 8, 2020
View reviewed changes
tests/test_random_affine.py Outdated Show resolved Hide resolved
Nic-Ma
Nic-Ma reviewed Mar 8, 2020
View reviewed changes
tests/test_affine.py Outdated Show resolved Hide resolved
Nic-Ma
Nic-Ma requested changes Mar 8, 2020
View reviewed changes
Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

Added several comments inline.
As this PR is very big, need to double-check the math computations carefully later.
Thanks.

@wyli
Copy link
Contributor Author

wyli commented Mar 8, 2020 •
edited
Loading

Added several comments inline.
As this PR is very big, need to double-check the math computations carefully later.
Thanks.

thanks! addressed most of the comments, the pr is too large to include a solution for the ordering of the operations

Nic-Ma
Nic-Ma approved these changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

Your transforms can support both CPU and GPU but all the unit tests only cover "device=None" or "CPU".
Could you please locally run all the unit tests on GPU to double confirm?
Others look good to me now.
As this PR is very big, I suggest waiting 1 more day for other guys to review.
Thanks.

monai/networks/layers/simplelayers.py Outdated Show resolved Hide resolved
@wyli wyli merged commit 7304182 into master Mar 9, 2020
@wyli wyli deleted the pt-spatial-transforms branch May 21, 2020 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericspod ericspod ericspod left review comments

@Nic-Ma Nic-Ma Nic-Ma approved these changes

@madil90 madil90 Awaiting requested review from madil90

@tvercaut tvercaut Awaiting requested review from tvercaut

@atbenmurray atbenmurray Awaiting requested review from atbenmurray

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

implement stacked spatial transforms as coordinates ops + resampling Port elastic deformation transform
3 participants
@wyli @ericspod @Nic-Ma