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.

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

Add missing initializer argument of 0 to tree_reduce in tree_vdot and tree_sum. #1065

Conversation

carlosgmartin
Copy link
Contributor

Currently, the following commands

$ python3 -c "import optax; optax.tree_utils.tree_sum({})"
$ python3 -c "import optax; optax.tree_utils.tree_vdot({}, {})"

yield the following error:

TypeError: reduce() of empty iterable with no initial value

This PR fixes this bug by setting initializer=0 in the call to tree_reduce inside each of these functions.

@vroulet
Copy link
Collaborator

vroulet commented Sep 19, 2024

Thanks! Can you add a quick test too? (just to bookkeep such edge cases in our tests in case we make a new similar function)

@carlosgmartin carlosgmartin force-pushed the add_missing_tree_reduce_initializer branch 2 times, most recently from dfb10c8 to 3ff23eb Compare September 20, 2024 02:04
@carlosgmartin
Copy link
Contributor Author

@vroulet Done.

Copy link
Collaborator

@vroulet vroulet left a comment

Choose a reason for hiding this comment

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

Thanks!

@carlosgmartin carlosgmartin force-pushed the add_missing_tree_reduce_initializer branch 2 times, most recently from 9aa8805 to 5e44d8a Compare September 21, 2024 04:27
@fabianp
Copy link
Member

fabianp commented Sep 23, 2024

thanks for the contribution @carlosgmartin !

do you mind to explicitly use the keyword argument ? that is, use

 jtu.tree_reduce(operator.add, vdots, initializer=0)

instead of the current jtu.tree_reduce(operator.add, vdots, 0). They're semantically equivalent but the first one is easier to read and less error-prone

@carlosgmartin carlosgmartin force-pushed the add_missing_tree_reduce_initializer branch from 5e44d8a to d2d30b8 Compare September 23, 2024 16:24
@carlosgmartin
Copy link
Contributor Author

@fabianp Done.

@copybara-service copybara-service bot merged commit 04d79e5 into google-deepmind:main Oct 1, 2024
8 checks passed
@carlosgmartin carlosgmartin deleted the add_missing_tree_reduce_initializer branch October 1, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants