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

Handle invalid reduction values #6675

Merged
merged 13 commits into from Oct 5, 2022
Merged

Handle invalid reduction values #6675

merged 13 commits into from Oct 5, 2022

Conversation

adityagandhamal
Copy link
Contributor

Fixes #6604

As stated, reduction methods are restricted to ["none", "mean", "sum"].
This PR contains modifications to the code which prompts the user a ValueError if an anonymous value is passed.

Also, added respective unit tests to support the changes.

@facebook-github-bot
Copy link

Hi @adityagandhamal!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@oke-aditya
Copy link
Contributor

Think a bit more. Can we make this bit more efficient? Are we doing unnecessary computation?

datumbox
datumbox reviewed Oct 3, 2022
View reviewed changes
Copy link
Contributor

@datumbox datumbox 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 PR @adityagandhamal. Glad you managed to solve the installation issues you phased. The overall approach looks good, I only have a couple of comments. See below:

torchvision/ops/focal_loss.py Outdated Show resolved Hide resolved
test/test_ops.py Outdated Show resolved Hide resolved
@adityagandhamal
Copy link
Contributor Author

@datumbox
I've done the changes as advised.

datumbox
datumbox previously approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for the contribution @adityagandhamal.

@oke-aditya thanks for the support!

I think the issue with the linter is unrelated. I will check again tomorrow to see if it's fixed. cc @pmeier

@pmeier
Copy link
Collaborator

pmeier commented Oct 4, 2022

I think the issue with the linter is unrelated. I will check again tomorrow to see if it's fixed.

It is and this was fixed in #6678. Updating the branch should be sufficient.

@datumbox datumbox dismissed their stale review October 4, 2022 21:59

Actual issue on linter masked by failing linter job

@datumbox
Copy link
Contributor

datumbox commented Oct 4, 2022

@pmeier Thanks a lot for having a look. I confirm the job runs now correctly.

@adityagandhamal Unfortunately there are indeed some linter issues. You can see them here, by clicking on the Required lint modifications section. You don't need to fix them manually, just run ufmt format . from the root of the project and the utility will fix it for you. Then commit and push and we should be good to go. :)

@oke-aditya
Copy link
Contributor

oke-aditya commented Oct 5, 2022 •
edited

While I agree with @datumbox and thanks a lot to @adityagandhamal for all the work!

Should we be optimizing for the case of returning the error sooner? This seems to me an unnecessary optimization which leads to an idiom that is not present anywhere else in the codebase.

IMO the silver bullet was using Enums. Which automatically prevents invalid reduction modes and would have returned error sooner as well as kept the code clean.

I think Enum was not used in core due to JIT.

That's fine let's accept the current solution as it too works. Anyways let's keep things simple and move ahead 😃

@adityagandhamal
Copy link
Contributor Author

just run ufmt format . from the root of the project and the utility will fix it for you. Then commit and push and we should be good to go. :)

Okay

datumbox
datumbox approved these changes Oct 5, 2022
View reviewed changes
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM (again)! Let's wait for green CI and merge. 😃

@adityagandhamal
Copy link
Contributor Author

Why do some of these tests fail given everything runs fine on my end?

@oke-aditya
Copy link
Contributor

If They are not be caused by your PR you don't need to worry. There may be some other changes / many reasons for Failures.

@datumbox datumbox merged commit 96d1fec into pytorch:main Oct 5, 2022
@datumbox
Copy link
Contributor

datumbox commented Oct 5, 2022

All good. The CI failure is unrelated. Thanks for your work!

@adityagandhamal
Copy link
Contributor Author

Thank you!
Grateful to all for the support. Looking forward to contributing more in the future.

facebook-github-bot pushed a commit that referenced this pull request Oct 7, 2022
@NicolasHug @pmeier
@datumbox @facebook-github-bot
[fbsync] Handle invalid reduction values (#6675)
ac0cef0 
Summary:
* Add ValueError

* Add tests for ValueError

* Add tests for ValueError

* Add ValueError

* Change to if/else

* Ammend iou_fn tests

* Move code excerpt

* Format tests

Reviewed By: datumbox

Differential Revision: D40138724

fbshipit-source-id: 56c742a8c2ff80f2f51cba4cb3156835ed250653

Co-authored-by: Philip Meier <github.pmeier@posteo.de>
Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oke-aditya oke-aditya oke-aditya left review comments

@datumbox datumbox datumbox approved these changes

Assignees
No one assigned
Labels
cla signed enhancement module: ops
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Throw ValueError for wrong reduction in losses
5 participants
@adityagandhamal @facebook-github-bot @oke-aditya @pmeier @datumbox