-
Notifications
You must be signed in to change notification settings - Fork 17
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
implement gradient masking with cropping - new PR #101
Conversation
Thanks @JonasHell. I will have a look soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! The changes to loss_wrapper.py
all look good to me.
It would be good to extend the test cases a bit to cover all the changes:
- In
test_masking
: test for bothcrop
andmultiply
masking methods that the masked gradients are zero. (Can just put this in a loop whereApplyAndRemoveMask
is called with the corresponding argument.) The test could also be renamed totest_ApplyAndRemove...
to make the naming more consistent. - Add a test for
MaskIgnoreLabel
along the lines oftest_masking
.
With the extended tests this is good to go from my side.
Done :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests look good now. Thanks!
I cleaned my git repo. Unfortunately, renaming of branches leads to the closing of connected PRs. So I opened a new one. To not loose the old discussion, I link the old PR here.