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.

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

Fix lazy rand affine #6774

Merged
merged 6 commits into from
Jul 28, 2023
Merged

Fix lazy rand affine #6774

merged 6 commits into from
Jul 28, 2023

Conversation

function2-llx
Copy link
Contributor

@function2-llx function2-llx commented Jul 25, 2023

Fixes #6773.

Description

Call rand_affine_grid() once before call rand_affine_grid.get_transformation_matrix(), since its documented as "Get the most recently applied transformation matrix", or the .affine attribute will not be set.
Also, set randomize=False here since randomization if performed in the beginning of the function.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.

@function2-llx function2-llx marked this pull request as ready for review July 25, 2023 17:51
@function2-llx function2-llx changed the title [WIP] Fix lazy rand affine Fix lazy rand affine Jul 25, 2023
Signed-off-by: function2 <function2-llx@outlook.com>
@function2-llx
Copy link
Contributor Author

very sad, find this bug after training the model for 5 days

@ericspod
Copy link
Member

@atbenmurray could you please have a look (I'm away still)? Thanks!

@atbenmurray
Copy link
Contributor

Hi there @function2-llx , I'm sorry you've had a problem with this. I'll take a look at the proposed fix.

@function2-llx
Copy link
Contributor Author

Thank you for your concern, @atbenmurray. I understand that this is an open-source project and issues are part of the process. Let's continue to work together to improve this project!

@atbenmurray
Copy link
Contributor

Please forgive the delay. I've been looking into the code this morning. I think we might want to go a little further than the current PR, but I'll make suggestions in the review once I've come to a conclusion on that.

@atbenmurray
Copy link
Contributor

@function2-llx Ok, I had a look at a limited refactor of RandAffine and the classes that it uses, but I think it isn't possible to make tweaks without making a larger refactor. As such, I think we should go ahead with the change that you are proposing.

Do you want to write some unit tests to cover the change to RandAffine? We certainly need additional tests to cover this.

@function2-llx
Copy link
Contributor Author

@atbenmurray Hi, I'd like to help, but I have to rush my conference submission recently (Due on Aug. 15th), so I'm afraid that this PR is the most I can help recently. I can help more after the submission is finished.

@atbenmurray
Copy link
Contributor

@function2-llx Don't worry about it. It has been a very useful spot! I'll write up some additional unit tests to go with it.

@wyli
Copy link
Member

wyli commented Jul 28, 2023

/build

@wyli wyli enabled auto-merge (squash) July 28, 2023 21:01
Copy link
Member

@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.

integration verified, please feel free to create follow up PRs, thanks

@wyli wyli merged commit 5feb353 into Project-MONAI:dev Jul 28, 2023
35 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lazy RandAffine has no effect
4 participants