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

Using variable declared at a broader scope in a function is bad form #3800

Open
afairley opened this issue Mar 30, 2024 · 1 comment
Open
Labels
Priority: P2 - no schedule Best effort response and resolution. We have no plan to work on this at the moment.

Comments

@afairley
Copy link

afairley commented Mar 30, 2024

In person I would argue vociferously that this is kind of a major sin . tx should either be passed in as an argument or both tx and update_step should live on some object of which tx should be a variable. Maybe the neural network instance, that way all of the code in this tutorial can be typed into a single notebook without the later declaration of update_step shadowing the first one.

updates, opt_state = tx.update(grads, opt_state) # Defined below.

@afairley
Copy link
Author

Looking at https://github.com/google/flax/blob/main/docs/guides/flax_fundamentals/flax_basics.md way down the bottom, it looks like tx is usually(occasionally?) a parameter for update_step declarations. Anyways this is a bit of a nitpick, but I'm definitely not the only person strongly sensitized to this nit.

@chiamp chiamp added the Priority: P2 - no schedule Best effort response and resolution. We have no plan to work on this at the moment. label Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P2 - no schedule Best effort response and resolution. We have no plan to work on this at the moment.
Projects
None yet
Development

No branches or pull requests

2 participants