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 docstring parser to remove duplicate in Gymnasium website #329

Merged

Conversation

vcharraut
Copy link
Contributor

Description

As stated in #320, the documentation Gymnasium website contains duplicated content that can be confusing. Both the docstring from the Class and the __init__ are displayed, meaning there is often a sentence repeated twice.

To keep the documentation the same, I just added a parser to the Sphinx conf.py to remove any lines that are written before a Args in the docstring.

Example

class TransformReward(RewardWrapper):
    """Transform the reward via an arbitrary function.

    Warning:
        If the base environment specifies a reward range which is not invariant under :attr:`f`, the :attr:`reward_range` of the wrapped environment will be incorrect.

    Example:
        >>> import gymnasium as gym
        >>> from gymnasium.wrappers import TransformReward
        >>> env = gym.make("CartPole-v1")
        >>> env = TransformReward(env, lambda r: 0.01*r)
        >>> _ = env.reset()
        >>> observation, reward, terminated, truncated, info = env.step(env.action_space.sample())
        >>> reward
        0.01
    """

    def __init__(self, env: gym.Env, f: Callable[[float], float]):
        """Initialize the :class:`TransformReward` wrapper with an environment and reward transform function :attr:`f`.

        Args:
            env: The environment to apply the wrapper
            f: A function that transforms the reward
        """
        super().__init__(env)
        assert callable(f)
        self.f = f

Here the parser will remove the lines before Args, which are the whitespace and Initialize the :class:TransformReward wrapper with an environment and reward transform function :attr:f..

Important

The parser targets every docstring that has the tag class, which are the Class and __init__ ones. This means that if you place an Args section in the Class docstring, the lines before it will be removed it too.

I think only one class had this problem, but it should be noted as bad practice for the future.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@vcharraut vcharraut marked this pull request as ready for review February 14, 2023 23:07
Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

@mgoulao Could you confirm that this is a good idea?
Otherwise, looks good to merge

@mgoulao
Copy link
Contributor

mgoulao commented Feb 15, 2023

Why dont we remove the docstring under __init__ and move Args to the class docstring?

Example

class TransformReward(RewardWrapper):
    """Transform the reward via an arbitrary function.
   
    Args:
        env: The environment to apply the wrapper
        f: A function that transforms the reward 
  
    Warning:
        If the base environment specifies a reward range which is not invariant under :attr:`f`, the :attr:`reward_range` of the wrapped environment will be incorrect.

    Example:
        >>> import gymnasium as gym
        >>> from gymnasium.wrappers import TransformReward
        >>> env = gym.make("CartPole-v1")
        >>> env = TransformReward(env, lambda r: 0.01*r)
        >>> _ = env.reset()
        >>> observation, reward, terminated, truncated, info = env.step(env.action_space.sample())
        >>> reward
        0.01
    """

    def __init__(self, env: gym.Env, f: Callable[[float], float]):
        super().__init__(env)
        assert callable(f)
        self.f = f

@pseudo-rnd-thoughts
Copy link
Member

@mgoulao As we have pydocstyle in pre-commit and this requires minimal changes to the code base + ide will still work as expected

@mgoulao
Copy link
Contributor

mgoulao commented Feb 15, 2023

I'm just afraid that we are creating too much complexity and that might exist some edge cases that we are not seeing.

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

Generating the website locally, I don't see any issues with the implementation and I agree that it makes it easier to read.

If there is an issue we can revert this change for a future release.

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit 5bb67ee into Farama-Foundation:main Feb 15, 2023
@vcharraut
Copy link
Contributor Author

I'm just afraid that we are creating too much complexity and that might exist some edge cases that we are not seeing.

I agree with the statement because it is kinda a hack, but I didn't find a better way to fix it without changing the current docstring format. This can be still discussed to change the format of the docstring (removing __init__ content or moving Args to the Class docstring), but then it should affect every .py files

@vcharraut vcharraut deleted the add-documentation-parser branch February 15, 2023 20:08
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