Skip to content

Croppad inverse transform#1737

Merged
wyli merged 18 commits intoProject-MONAI:masterfrom
rijobro:croppad_inverse_transform
Mar 13, 2021
Merged

Croppad inverse transform#1737
wyli merged 18 commits intoProject-MONAI:masterfrom
rijobro:croppad_inverse_transform

Conversation

@rijobro
Copy link
Copy Markdown
Contributor

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

Croppad inverse transforms. All lossless (thus all the 0's in test_inverse.py).

#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 --codeformat --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

rijobro added 3 commits March 10, 2021 17:06
@rijobro
croppad inverse
15a350a 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
improve decollate test for set_determinism
40322d7 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
lossless
4cf5779 
Signed-off-by: Richard Brown <[email protected]>
@rijobro rijobro requested review from Nic-Ma and wyli and removed request for Nic-Ma March 10, 2021 17:13
@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Mar 10, 2021 •
edited
Loading

@wyli as requested in #1663.

Nic-Ma
Nic-Ma approved these changes Mar 11, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Looks good to me.
Thanks.

Nic-Ma
Nic-Ma reviewed Mar 11, 2021
View reviewed changes
rijobro added 3 commits March 11, 2021 15:07
@rijobro
test lossless
f5f936b 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
Merge remote-tracking branch 'rijobro/croppad_inverse_transform' into…
a08a0a2 
… croppad_inverse_transform

Signed-off-by: Richard Brown <[email protected]>
@rijobro
code format
d8bbee1 
Signed-off-by: Richard Brown <[email protected]>
@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 11, 2021

/integration-test

wyli
wyli reviewed Mar 11, 2021
View reviewed changes
@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Mar 13, 2021

@wyli if you're happy with the PR now, would you mind enabling auto-merge?

@wyli wyli enabled auto-merge (squash) March 13, 2021 11:21
wyli
wyli approved these changes Mar 13, 2021
View reviewed changes
Copy link
Copy Markdown
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, this looks good to me!

@wyli wyli merged commit f56058d into Project-MONAI:master Mar 13, 2021
@rijobro rijobro deleted the croppad_inverse_transform branch March 13, 2021 14:03
@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 14, 2021

there's an assertion error that occurs randomly...

  File "/__w/MONAI/MONAI/tests/test_inverse.py", line 291, in check_inverse
    self.assertLessEqual(mean_diff, acceptable_diff)
AssertionError: 0.47619047619047616 not less than or equal to 0

Failed: RandSpatialCropd1D odd Compose. Mean diff = 0.47619047619047616 (expected <= 0), unmodified diff: 2.0952380952380953

@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Mar 14, 2021

This could happen if the part that is randomly cropped includes non-zero voxels. But given that the seed is used during setUp, I would have thought that if it passed the test once, it would always pass?

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 14, 2021 •
edited
Loading

yes, strange that I couldn't replicate the issue locally on Linux or mac os. I've seen the errors from the CI a few times, the mean diff is always 0.4761... though

This was referenced Mar 15, 2021
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@wyli wyli wyli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rijobro @wyli @Nic-Ma