I can see why we might want to change the C code, but do we really think that breaking backward compatibility to make people say reduce=Reduce.NONE instead of reduce=False is a good idea? It's certainly less Pythonic. The new thing is much more verbose, and the old API seemed to work quite well.

In Python-land, maybe "reduce=None" should default to reduce=Reduce.NONE?

This current version won't break backwards compatibility, but I was planning to stop supporting it in a future version.

A couple motivators behind this change:

• size_average and reduce don't act independently of each other, this seems like bad API design.
• we want to add more support for other times of reduction, like average by dividing by batch size instead of number of elements. I don't think it's a good idea to keep adding more boolean flags

One more random idea: deprecate size_average and reduce, and introduce a new argument (maybe reduction? Idk)?
While I'm ok with reduce, it seems to be a special keyword in python and gets coloured differently. It might also make for a simpler deprecation path.

reduce is not a keyword, but a name of a builtin that was actually removed in Python 2 (it's no longer imported by default).

I really don't think that using enums is very user friendly. It's definitely more verbose, and very rare in Python APIs. What's the issue with the two boolean flags we have right now? Sure, there are configurations which are incompatible, but assuming we keep our current defaults, you only specify at most one of those two arguments, and no issues should arise.

I think there is value removing both boolean flags and keeping only a single one.
I'd say that the behavior of size_average is unexpected in a few cases, and this could be a way of fixing it without breaking backward compatibility.

I also agree that enum is not very pythonic: what about having strings instead?

We also want to add support for different types of averaging (dividing by batch_size instead of number of elements). Under the current scheme the only way to do that is to add a third boolean argument, which again, will only be valid if the others are in certain configurations.

I think combining the arguments makes a lot of sense if we look at the perspective of a user: Most people are going to be looking at the documentation before using any of these functions, and I think the documentation would be a simpler to understand if all these similar functionalities were under a single argument.

I definitely have been confused by what certain combination would do, so at least in the front of making the behavior clearer to users, this certainly helps.

That said, enum indeed isn't really used in Python API, as @apaszke said. A more common pattern is to directly use strings, as we are already doing in F.pad.

Strings aren’t ideal, but they’re better than enums. I would be ok with that

@pytorchbot retest this please

@pytorchbot retest this please

Please fix tests and rebase too

@pytorchbot retest this please

@pytorchbot retest this please

@ssnl You're talking about Reduction in nn/functional.py, right? Should that still be prefixed if I'm using it in other files as well?

@li-roy yes, because that's not part of public api. In other words, it shouldn't show up in user's IDE code autocompletion.

@ssnl Oh that makes sense now, thanks for the explanation.

btw, if you haven't, you should rebase & import to phabricator.