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

Improved param and rx docs #977

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

Improved param and rx docs #977

wants to merge 28 commits into from

Conversation

MarcSkovMadsen
Copy link
Collaborator

@MarcSkovMadsen MarcSkovMadsen commented Nov 13, 2024

Attempt to make .param, .rx and .rx() easier to use by improving docstrings.

Currently I meet a lot of dead ends because of missing or very sparse docstrings.

For example .rx in example below has no docstring which makes it almost impossible to use without already knowing how.

import param

class MyClass(param.Parameterized):
    value = param.Parameter()

my_class = MyClass(value=0)
rx_value = my_class.param.value.rx

Its the same for .param. There is no docstring.

I do believe it would help users tremendously if there where docstring and improved docstrings. It would make the .param and .rx namespaces so much more inviting and easy to use.

Guiding Principles

  • Use Numpy style docstrings (would strongly prefer google docstrings because they are better as tooltips)
  • Add types where they are easy to add
  • Document in style similar to Panel docstrings
    • Reference link
    • Basic examples that can run
  • Ok to make smaller refactors of code if they can be done without breaking changes.

@MarcSkovMadsen MarcSkovMadsen changed the title Improved rx docs Improved param and rx docs Nov 13, 2024
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.36%. Comparing base (1868240) to head (3237360).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #977   +/-   ##
=======================================
  Coverage   87.35%   87.36%           
=======================================
  Files           9        9           
  Lines        4935     4938    +3     
=======================================
+ Hits         4311     4314    +3     
  Misses        624      624           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@MarcSkovMadsen
Copy link
Collaborator Author

As I am not a regular contributor to param it would help me to get a review, feedback and merge of this "smaller" PR.

Then I would feel confident making a larger PR more systematically going through .param and .rx namespaces and improving the docstrings and types.

Copy link
Member

@hoxbro hoxbro 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 so much! Love it.

.gitignore Outdated Show resolved Hide resolved
param/parameterized.py Show resolved Hide resolved
param/reactive.py Show resolved Hide resolved
Copy link
Member

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

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

Let's see if @philippjfr has anything to say.

Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

I found inconsistencies in the formatting of the docstrings added in this PR. I also wonder if the links shouldn't point to the reference API instead of to the user guides.

Before merging, I'd recommend building the dev site from this branch and checking that the generated API ref is correctly formatted.

param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/reactive.py Outdated Show resolved Hide resolved
Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

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

This is well overdue, so thanks for doing it. Let's make sure that we consistently use NumPy style docstrings though, right now this is an odd mix.

@MarcSkovMadsen
Copy link
Collaborator Author

This is well overdue, so thanks for doing it. Let's make sure that we consistently use NumPy style docstrings though, right now this is an odd mix.

If think I should investigate whether ruff or something else can validate this. I'm not using Numpy Docstrings very much. So I will fail at this.

@MarcSkovMadsen
Copy link
Collaborator Author

Thanks so much for the feedback. Will correct it ASAP.

@MarcSkovMadsen
Copy link
Collaborator Author

This is well overdue, so thanks for doing it. Let's make sure that we consistently use NumPy style docstrings though, right now this is an odd mix.

I've updated the docstrings. Also means the code blocks are now >>> prefixed.

Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

Thanks for the updates Marc!

I see that Panel also has its examples code doctest formatted (>>> <code>). What's your experience using this format? I know that it sometimes annoys me as copy/pasting it (from a website, a docstring tooltip in an IDE, etc.) doesn't always strip >>> so you have to manually remove them. IIRC the ipython interpreter strips them out but not the base python REPL. The advantage of this format is that it enables running doctest which might be a good idea. By the way, I have no strong opinion over all of that, just trying to gauge the user experience.

param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Nov 15, 2024

I also think they are hard to copy. But I changed them because of request to use numpy docstrings. I believe that is the format.

@MarcSkovMadsen
Copy link
Collaborator Author

It is the format

image

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Nov 16, 2024

I believe I got the feedback from @maximlt above that >>> codestyle is not very helpful as its hard to copy examples. I agree.

So even though >>> is numpy docstyle. I plan to overrule this by restructuredText code blocks. Please let me know if you disagree @hoxbro , @philippjfr, @maximlt as implementing one style and then have to change it is a massive task. Thanks.

Alternative would be markdown code blocks. But numpy docstyle is rst.

@maximlt
Copy link
Member

maximlt commented Nov 16, 2024

So even though >>> is numpy docstyle. I plan to overrule this by restructuredText code blocks.

Could you show what that format looks like in the code? I'd also like to know how it will end up being displayed:

  • in VSCode
  • in JupyterLab
  • on the reference API of Param's website (you could just make a change to your branch, trigger a dev doc build from Github, and post a screenshot)

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Nov 16, 2024

So even though >>> is numpy docstyle. I plan to overrule this by restructuredText code blocks.

Could you show what that format looks like in the code? I'd also like to know how it will end up being displayed:

  • in VSCode
  • in JupyterLab
  • on the reference API of Param's website (you could just make a change to your branch, trigger a dev doc build from Github, and post a screenshot)

I'm simply not prepared to do that work again. I tried to analyze situation when started doing this for Panel. And the learning is there is no way that works great. If you are happy with Panel I'll do it like that. That is what I can offer.

Panel seems to be using >>> from sampling 4 modules.

@maximlt
Copy link
Member

maximlt commented Nov 17, 2024

I'm simply not prepared to do that work again. I tried to analyze situation when started doing this for Panel. And the learning is there is no way that works great. If you are happy with Panel I'll do it like that. That is what I can offer.

Ok sure I understand, this is all a bit of a mess.

So even though >>> is numpy docstyle. I plan to overrule this by restructuredText code blocks.

Could you show how you plan to change it with an example? For example, how will this docstring look like with the change you plan to make?

        """
        Return the serialized parameters of the Parameterized object.

        Parameters
        ----------
        subset : list, optional
            A list of parameter names to serialize. If None, all parameters will be serialized. Defaults to None.
        mode : str, optional
            The serialization format. By default, only 'json' is supported. Defaults to 'json'.

        Returns
        -------
        Any
            The serialized value.

        User Guide
        ----------
        https://param.holoviz.org/user_guide/Serialization_and_Persistence.html#serializing-with-json

        Examples
        --------
        Create a Parameterized instance and serialize its parameters:

        >>> import param
        >>> class P(param.Parameterized):
        >>>     a = param.Number()
        >>>     b = param.String()
        >>> p = P(a=1, b="hello")

        Serialize parameters:

        >>> serialized_data = p.param.serialize_parameters()
        >>> print(serialized_data)
        {"name": "P00002", "a": 1, "b": "hello"}
        """

Then I can look a little bit what would the user experience be on various platforms (VSCode, JLab, website, etc.).


Just linking something that I wasn't aware of, the sphinx-copybutton Sphinx extension has lots of ways to configure what gets copied in the end, possibly stripping out >>> : https://sphinx-copybutton.readthedocs.io/en/latest/use.html. I'm not sure why Pandas doesn't use that (check https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.sort_values.html).

Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

This looks very useful; thanks @MarcSkovMadsen !

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Nov 23, 2024

I'm simply not prepared to do that work again. I tried to analyze situation when started doing this for Panel. And the learning is there is no way that works great. If you are happy with Panel I'll do it like that. That is what I can offer.

Ok sure I understand, this is all a bit of a mess.

So even though >>> is numpy docstyle. I plan to overrule this by restructuredText code blocks.

Could you show how you plan to change it with an example? For example, how will this docstring look like with the change you plan to make?

        """
        Return the serialized parameters of the Parameterized object.

        Parameters
        ----------
        subset : list, optional
            A list of parameter names to serialize. If None, all parameters will be serialized. Defaults to None.
        mode : str, optional
            The serialization format. By default, only 'json' is supported. Defaults to 'json'.

        Returns
        -------
        Any
            The serialized value.

        User Guide
        ----------
        https://param.holoviz.org/user_guide/Serialization_and_Persistence.html#serializing-with-json

        Examples
        --------
        Create a Parameterized instance and serialize its parameters:

        >>> import param
        >>> class P(param.Parameterized):
        >>>     a = param.Number()
        >>>     b = param.String()
        >>> p = P(a=1, b="hello")

        Serialize parameters:

        >>> serialized_data = p.param.serialize_parameters()
        >>> print(serialized_data)
        {"name": "P00002", "a": 1, "b": "hello"}
        """

Then I can look a little bit what would the user experience be on various platforms (VSCode, JLab, website, etc.).

Just linking something that I wasn't aware of, the sphinx-copybutton Sphinx extension has lots of ways to configure what gets copied in the end, possibly stripping out >>> : https://sphinx-copybutton.readthedocs.io/en/latest/use.html. I'm not sure why Pandas doesn't use that (check https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.sort_values.html).

The below is how it looks in my version of VS Code on windows with pylance. I don't have the latest version of VS Code.

For me that is fine. It looks good. The >>> and ... are pasted too. But that is a Feature Request for VS Code/ Pylance to fix. We are using the "standard" numpy format.

vs-code.mp4

VS Code and jedi-language-server actually works better because it strips out the >>> and .... Both when shown and when copy-pasted. This is perfect.

vscode-jedi-language-server.mp4

Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

LGTM other than 1 pending suggestion. Let's preserve the original Numpy style for the examples, I think we can tweak the sphinx-copybutton extension to work in a decent way, and hopefully VSCode will make it easier to copy/paste these snippets. If we end up needing to change the style, it shouldn't be too hard to write a script to re-write the docstrings automatically, stripping >>> and ... (the other way around, yes!).

Comment on lines +2320 to +2321
>>> a = param.Number()
>>> b = param.String()
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants