Skip to content

Updated "optional arguments" section. #250

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Updated "optional arguments" section. #250

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6cd0c88
Updated "optional arguments" section.
dynamicwebpaige May 28, 2020
b1caad6
Update api-reviews.md
dynamicwebpaige Aug 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions governance/api-reviews.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,18 @@ imperfections into new APIs for the sake of consistency with old APIs.

### Optional arguments with default values

Many APIs have optional arguments with a default value. Our recommendation is to
use `None` as the default value of any optional arguments and have the
implementation be responsible for handling it as opposed to using a default
value that directly represents the behavior (e.g. `aggregate='sum'`). The
latter prevents the implementation from distinguishing between the caller not
setting the argument vs. the caller setting the argument to the default value,
which may be needed when the default behavior is changing.
Many APIs have optional arguments that assign a default value (for example, `activation = None`
in [`tf.keras.layers.Dense`](https://www.tensorflow.org/api_docs/python/tf/keras/layers/Dense)).

Our recommendation is to use `None` as the default value for _any optional arguments
that may be adjusted or changed over time_, and have the implementation be responsible
for handling the value, as opposed to using a default value that directly represents
the behavior (e.g. `aggregate='sum'`). The latter prevents the implementation from
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention num_threads=10 instead of aggregate=sum? That's more in keeping with stuff that can obviously change (changing the aggregation is incompatible)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed!

distinguishing between the caller not setting the argument vs. the caller setting the
argument to the default value, which may be needed when the default behavior is changing.

If the optional argument is _backwards incompatible to change_, however, its default should
reflect the actual default value when possible.
Copy link

Choose a reason for hiding this comment

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

Can we include @fchollet 's Milk example as well? I rather like that as a way to differentiate the two. In code would be ideal, so we can show what we mean by letting the implementation set the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

The activation function or aggregation is a good example of something backwards incompatible and should be here


### Does it belong in TF at all?

Expand Down