optax icon indicating copy to clipboard operation
optax copied to clipboard

Fix NamedTuples Formatting

Open amosyou opened this issue 1 year ago • 8 comments

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

amosyou avatar Feb 15 '24 09:02 amosyou

@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)

amosyou avatar Feb 15 '24 09:02 amosyou

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).

vroulet avatar Feb 15 '24 21:02 vroulet

@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?

amosyou avatar Feb 21 '24 04:02 amosyou

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

vroulet avatar Feb 21 '24 07:02 vroulet

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)

vroulet avatar Feb 21 '24 07:02 vroulet

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

amosyou avatar Mar 15 '24 18:03 amosyou

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?

amosyou avatar Apr 17 '24 04:04 amosyou

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

fabianp avatar Apr 17 '24 07:04 fabianp