Skip to content

[1798] fix RandZoomd collation#1801

Merged
bhashemian merged 18 commits intoProject-MONAI:masterfrom
rijobro:RandZoomd_fix
Mar 18, 2021
Merged

[1798] fix RandZoomd collation#1801
bhashemian merged 18 commits intoProject-MONAI:masterfrom
rijobro:RandZoomd_fix

Conversation

@rijobro
Copy link
Copy Markdown
Contributor

@rijobro rijobro commented Mar 18, 2021

Fixes #1798.

Description

self._zoom was handled differently depending on self._do_transform. Modified to be the same, such that they can be collated together.

Also add testing for all invertible random transforms.

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.
  • Documentation updated, tested make html command in the docs/ folder.

Signed-off-by: Richard Brown <[email protected]>
@rijobro rijobro requested review from Nic-Ma and wyli March 18, 2021 10:47
@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Mar 18, 2021

@behxyz, do you think you could try this PR? I'm fairly confident it will have solved the problem.

@rijobro rijobro added the bug Something isn't working label Mar 18, 2021
@rijobro rijobro changed the title fix RandZoomd collation [1798] fix RandZoomd collation Mar 18, 2021
rijobro added 2 commits March 18, 2021 11:07
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
rijobro added 7 commits March 18, 2021 11:20
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
rijobro added 3 commits March 18, 2021 15:30
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
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 for the quick fix!

@rijobro rijobro enabled auto-merge (squash) March 18, 2021 15:40
@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Mar 18, 2021

@wyli do you understand the error? https://github.com/Project-MONAI/MONAI/pull/1801/checks?check_run_id=2141079751#step:11:5166

It seems that the following code is being executed on the windows and osx test:

# skip this test if multiprocessing uses 'spawn', as the check is only basic anyway
if torch.multiprocessing.get_start_method(allow_none=True) == "spawn":
    # Check that error is thrown when inverse are used out of order.
    t = SpatialPadd("image", [10, 5])
    with self.assertRaises(RuntimeError):
        t.inverse(forwards[-1])  # <-- windows and osx shouldn't reach here, but they do, causing error

wyli and others added 3 commits March 18, 2021 16:12
Signed-off-by: Wenqi Li <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: Richard Brown <[email protected]>
@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Mar 18, 2021

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 18, 2021

@wyli do you understand the error? https://github.com/Project-MONAI/MONAI/pull/1801/checks?check_run_id=2141079751#step:11:5166

It seems that the following code is being executed on the windows and osx test:

# skip this test if multiprocessing uses 'spawn', as the check is only basic anyway
if torch.multiprocessing.get_start_method(allow_none=True) == "spawn":
    # Check that error is thrown when inverse are used out of order.
    t = SpatialPadd("image", [10, 5])
    with self.assertRaises(RuntimeError):
        t.inverse(forwards[-1])  # <-- windows and osx shouldn't reach here, but they do, causing error

I've changed allow_none to False, it'll set a specific start method otherwise I think it's None...

@bhashemian bhashemian disabled auto-merge March 18, 2021 17:34
@bhashemian
Copy link
Copy Markdown
Member

@behxyz, do you think you could try this PR? I'm fairly confident it will have solved the problem.

Sure, let me check this PR before merging.

@bhashemian
Copy link
Copy Markdown
Member

It works perfectly! Thank you, @rijobro, for the quick fix.

@bhashemian bhashemian enabled auto-merge (squash) March 18, 2021 17:40
@bhashemian bhashemian merged commit 86cbf05 into Project-MONAI:master Mar 18, 2021
@rijobro rijobro deleted the RandZoomd_fix branch March 18, 2021 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PR #1778 causes error

3 participants