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

Batch inverse #1795

Merged
merged 27 commits into from
Mar 20, 2021
Merged

Batch inverse #1795

merged 27 commits into from
Mar 20, 2021

Conversation

rijobro
Copy link
Contributor

@rijobro rijobro commented Mar 17, 2021 •
edited by wyli
Loading

Batch inverse.

part of #1515

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • 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.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@rijobro rijobro requested review from wyli, ericspod and Nic-Ma March 17, 2021 16:46
@rijobro rijobro mentioned this pull request Mar 17, 2021
Merged
6 tasks
rijobro added 6 commits March 17, 2021 17:07
@rijobro
update
0682344 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
autofix
b7aec08 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
Merge remote-tracking branch 'MONAI/master' into batch_inverse
de51114 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
Merge remote-tracking branch 'MONAI/master' into batch_inverse
77ff3cd 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
cant pickle lambda
b78ee22 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
No ID check for windows
dac89ba 
Signed-off-by: Richard Brown <[email protected]>
@rijobro rijobro added the enhancement New feature or request label Mar 18, 2021
rijobro added 6 commits March 18, 2021 12:17
@rijobro
win32 change
2d1ca33 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
autofix
0a8d321 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
code format
385b9ce 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
more changes
a03ce5f 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
changes
ba6ecbb 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
Merge remote-tracking branch 'MONAI/master' into batch_inverse
a7cf63e
@rijobro
Copy link
Contributor Author

rijobro commented Mar 19, 2021

Hi @Nic-Ma, @wyli, I know it takes a lot of time, but I would really appreciate reviews on this PR, #1793 and #1794 so that I can move onto something else! Thanks in advance

@wyli
Copy link
Contributor

wyli commented Mar 19, 2021

yes, I'm looking into these... @Nic-Ma is taking a few days off, I think he'll be back next Tuesday

@rijobro
Copy link
Contributor Author

rijobro commented Mar 19, 2021

great, thanks!

wyli
wyli reviewed Mar 19, 2021
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.

thanks! I put some comments inline, still need more time to think and test this. I'm not very familiar with the use cases, I'll look into #1794 to get a complete picture...

monai/data/inverse_batch_transform.py Outdated
transform: InvertibleTransform,
pad_collation_used: bool,
) -> None:
self.data = decollate_batch(data)
Copy link
Contributor

@wyli wyli Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

could you help add some tests for random transforms in test_decollate with random prob=0.0?

also, not in this PR but came across an issue in monai.transforms.Decollated that missing a call to super().__init__

_BatchInverseDataset should call super() in its constructor as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests and added call to super constructors

monai/data/inverse_batch_transform.py Outdated
for key in data.keys():
transform_key = str(key) + "_transforms"
transform = data[transform_key][-1]
if transform["class"] == "SpatialPadd":
Copy link
Contributor

Choose a reason for hiding this comment

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

this is coupled with the pad_list_data_collate, could we make a special invertible transformPadListDataCollate, so that we could have the __call__ and inverse in one place...what do you think?

Copy link
Contributor Author

@rijobro rijobro Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Done and split into its own PR. Let's merge that first, which will simply the diff of this PR again.

@rijobro rijobro mentioned this pull request Mar 19, 2021
Merged
6 tasks
wyli
wyli approved these changes Mar 19, 2021
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.

thanks, looks good to me now.

@rijobro rijobro enabled auto-merge (squash) March 19, 2021 23:12
@rijobro rijobro merged commit a2aeb40 into Project-MONAI:master Mar 20, 2021
@rijobro rijobro deleted the batch_inverse branch March 20, 2021 13:54
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

@ericspod ericspod Awaiting requested review from ericspod

@Nic-Ma Nic-Ma Awaiting requested review from Nic-Ma

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rijobro @wyli