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

Fix NamedTuples Formatting #803

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

amosyou
Copy link
Contributor

@amosyou amosyou commented Feb 15, 2024

fixes duplicate formatting for classes inheriting from NamedTuples as seen in #669

@amosyou
Copy link
Contributor Author

amosyou commented Feb 15, 2024

@vroulet most of the classes / states in transform.py and combine.py don't have proper docstrings. would it still be worthwhile to remove :members: for those files (many of those classes won't have much rendered bc of no docstring)

@vroulet
Copy link
Collaborator

vroulet commented Feb 15, 2024

Hello @amosyou (sorry got too many mails didn't catch this notification). It's better not to have a docstring than having a confusing one. So yes, you can remove :members: to all classes and we may fill all the docstrings later (one file at a time probably).

@amosyou amosyou marked this pull request as ready for review February 21, 2024 04:16
@amosyou
Copy link
Contributor Author

amosyou commented Feb 21, 2024

@vroulet i removed the :members: from all the states with the exception of SAMState in sam.py since SAMState uses the chex.dataclass decorator instead of inheriting from NamedTuple. for consistency should we change that to be a namedtuple?

@vroulet
Copy link
Collaborator

vroulet commented Feb 21, 2024

That's great, it will really make the doc more readable!
For SAM's State let me handle that internally.

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.

First few comments, I'm going to build the doc and see for the pytypes.

optax/_src/constrain.py Outdated Show resolved Hide resolved
optax/_src/wrappers.py Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you made a pass on all States here to have a nice formatting you can keep the members attribute.
My idea was:

  • if you're brave and you change the docstring such that it's properly formatted, then let's keep :members: for that state.
  • otherwise, simply remove :members: and we may take care of the formatting of the states later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good! i brought back the members

@vroulet
Copy link
Collaborator

vroulet commented Feb 21, 2024

Could you also merge with main to see latest changes? (for example I'm not sure why States such as ClipState are displayed as alias of tuple instead of alias of EmptyState)

@amosyou
Copy link
Contributor Author

amosyou commented Mar 15, 2024

@vroulet apologies on the delay! been quite a month for me but we're back. ready for another review :)

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.

Thank you very much @amosyou. It's close to be good!
I have (hopefully last) comments:

  • Remove :members: from SAMState
  • Remove :members: of all classes in optimizer_wrappers.rst (I checked it works, that is we still have the handwritten attributes even after removing members)

@amosyou
Copy link
Contributor Author

amosyou commented Apr 17, 2024

apologies for the delay! It looks like a lot of restructuring happened since my last changes. I can open a new pull request that does these changes on the new transform subpackage?

@fabianp
Copy link
Member

fabianp commented Apr 17, 2024

thanks @amosyou ! Yes, that's fine, do it however way its most convenient for you 😄

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