Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Feature] Add reduction parameter to On-Policy losses. #1890
[Feature] Add reduction parameter to On-Policy losses. #1890
Changes from 12 commits
670108a
668d212
e624083
26cd568
3b253fc
81e6a3a
8560e8b
e747cec
b7c249c
ea93914
666e7b1
b5ef409
6275f02
107e875
3163d1d
331bd38
61fc41b
6dbb622
2d6674e
95efebd
e64ee3d
5368bdc
efaa893
ac115a3
c218352
eebcbb4
7e516f8
2701bb8
566b2b9
7e6b4b2
8052e33
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I would use
None
as a default, it can come in handy if some day we change the defaultsmth like
if reduction is None: reduction="mean"
later in the constructor.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.
I am not sure if I follow. The way I thought about it is that
mean
as default ensures BC. Also Torch losses havemean
as default, which I think is consistent. So it seemed to me a reasonable choice.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.
It will still be "mean". But not having "mean" is good practice because it allows us to detect whenever the user is passing or not the reduction.
Might come in handy at some point: for instance, if we want to change the default for one loss someday, we will just have to check whether the reduction is None or not.
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.
and how do you define a no-reduction option then?
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.
"none"
(notNone
)