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

Enhance documentation of param namespace #997

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

Conversation

MarcSkovMadsen
Copy link
Collaborator

@MarcSkovMadsen MarcSkovMadsen commented Dec 28, 2024

Continues from #992. Please review and merge that one first.

Focus is on updating the docstrings of Parameters class and its methods, i.e. the .param namespace.
I've added type annotations too.

Copy link

codecov bot commented Dec 28, 2024

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.24%. Comparing base (210419d) to head (d4fd0fc).

Files with missing lines Patch % Lines
param/parameterized.py 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #997      +/-   ##
==========================================
- Coverage   87.25%   87.24%   -0.02%     
==========================================
  Files           9        9              
  Lines        4928     4931       +3     
==========================================
+ Hits         4300     4302       +2     
- Misses        628      629       +1     

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

# Please update the docstring with better description and examples
# I've (MarcSkovMadsen) not been able to understand this. Its probably because I lack context.
# Its not mentioned in the documentation.
# The pytests do not make sense to me.
def set_dynamic_time_fn(self_,time_fn,sublistattr=None):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope someone else than me will update the docstring. I've not been able to understand this method. Its probably because I lack context. Its not mentioned in the documentation. The pytests are not valuable to me.

@@ -2815,6 +3178,9 @@ def values(self_, onlychanged=False):
vals.sort(key=itemgetter(0))
return dict(vals)

# Please update the docstring with better description and examples
# I've (MarcSkovMadsen) not been able to understand this. Its probably because I lack context.
# Its not mentioned in the documentation or pytests
def force_new_dynamic_value(self_, name): # pylint: disable-msg=E0213
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope someone else than me will update this docstring. I lack context and cannot find it in the documentation or pytests.

@MarcSkovMadsen MarcSkovMadsen marked this pull request as ready for review December 29, 2024 06:17
@MarcSkovMadsen
Copy link
Collaborator Author

I don't know why this fails on windows + python 3.12. Because this downstream PR #998 succeeds.

@maximlt
Copy link
Member

maximlt commented Jan 2, 2025

I don't know why this fails on windows + python 3.12

These are unrelated test failures.

@MarcSkovMadsen can you resolve the conflicts?

@philippjfr
Copy link
Member

Your efforts on these PRs is heroic, but please make PRs like this self-contained in future the merge conflicts are a nightmare.

@maximlt
Copy link
Member

maximlt commented Jan 2, 2025

Thanks @philippjfr !!! 🙏 I didn't feel brave enough to deal with these crazy conflicts on my first day back from holidays :D

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Jan 2, 2025

Your efforts on these PRs is heroic, but please make PRs like this self-contained in future the merge conflicts are a nightmare.

I would also like to avoid merge conflicts. But technically I don't know how to do this if one PR has to build on another PR. I was asked not to make one big PR.

  • How can I do this differently?
  • At work this almost always work. Why does it not work here?
  • As far as I can see the merge conflicts have been resolved (thanks) and I don't have to do anything.

@philippjfr
Copy link
Member

I was asked not to make one big PR.

I think this is quite context dependent, I would certainly have suggested that improving docstrings should have been a single PR, though separate PRs would have been fine if they didn't build on each other. For code I get the fact that each PR has to build on the other but I'm a little perplexed why that would be needed for docstrings. Certainly I would never suggest creating more than two separate PRs that depend on each other because you end up with this awful daisy chain of merge conflicts. This was probably the worst case scenario of that because the main issues were in the base PR, i.e. the Ruff PR with all the wrongly formatted docstrings. In any case, we're mostly done now.

@maximlt
Copy link
Member

maximlt commented Jan 2, 2025

Yes I suggested multiple PRs as I'm pretty sure that documenting some parts isn't going to be straightforward (e.g. documenting parameters.py without duplicating all the docstrings and making the module much longer to import) and I was concerned this would block merging any improvements at all! I also think we should give @jbednar a chance to chime in before merging all these docstring changes (even if it's just to say "go ahead!").

As far as I can see the merge conflicts have been resolved

FYI not in #998

Oh and I've just noticed you added type hints. It's nice but also more work for reviewers (shouldn't we set up mypy before adding type hints?). From a quick look, I saw for example that the docstring of watch shows that what is optional but the type hint doesn't. Which one is correct?

    def watch(
        self_, fn, parameter_names: list[str], what: str='value', onlychanged: bool=True,
        queued: bool=False, precedence: int=0
    ) -> Watcher:
        ...
        what : str, optional
            The type of change to watch for. By default, this is 'value', but it
            can be set to other slots such as 'constant'. Default is 'value'.

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Jan 2, 2025

I saw for example that the docstring of watch shows that what is optional but the type hint doesn't. Which one is correct?

    def watch(

        self_, fn, parameter_names: list[str], what: str='value', onlychanged: bool=True,

        queued: bool=False, precedence: int=0

    ) -> Watcher:

        ...

        what : str, optional

            The type of change to watch for. By default, this is 'value', but it

            can be set to other slots such as 'constant'. Default is 'value'.

Thx.

I think the type annotation and docstring is correct and consistent:

  • You dont type annotate that an argument has a default value and its optional to provide a value
  • the docstring "optional" does not refer to a type annotation. It means a default value has been set and that its optional to provide a value.

?

@maximlt
Copy link
Member

maximlt commented Jan 2, 2025

Ah true! I got confused with typing.Optional.

@philippjfr
Copy link
Member

As for the general typing question. I certainly would like to fully type param in the near future. I'm personally okay with adding a few types here or there until we start on that effort but if we want to add some basic mypy validation to the test suite first I'm also okay with that.

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.

Note that there are some comments I haven't repeated in all the places where they were relevant (e.g. when Returns is specified with None).

"""

def __init__(self_, cls, self=None):
def __init__(self_, cls: Type['Parameterized'], self: Union['Parameterized', None]=None):
Copy link
Member

Choose a reason for hiding this comment

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

Type[<>] is apparently deprecated since Python 3.9, you can use instead type[<>], see https://docs.python.org/3/library/typing.html#typing.Type

@@ -1933,7 +1938,29 @@ def watchers(self_, value):
self_.self._param__private.watchers = value

@property
def self_or_cls(self_):
def self_or_cls(self_) -> Union['Parameterized', Type['Parameterized']]:
Copy link
Member

Choose a reason for hiding this comment

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

self_or_cls is not documented on the website. It feels more like a helper for Param's internals, as far as I can see it's not used by other HoloViz libraries. Do you intend to make it more public? If not, I think we should make it private. Not sure it's worth the long docstring in that case, the code is straightforward.

Retrieve a Parameter by its key.

This method allows access to a class or instance Parameter using its name.
If accessed on an instance, the returned Parameter will be instantiated.
Copy link
Member

Choose a reason for hiding this comment

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

"will be instantiated" doesn't mean the same as "returns the instantiated parameter" to me. I understand it will be instantiated everytime __getitem__ will call, while the code only instantiates it (copies it really) if not already done.

'constructor before trying to access instance Parameter objects, or '
'looking up the class Parameter objects with `.param.objects(instance=False)` '
'may be enough for your use case.',
'Cannot access instance parameters before the Parameterized instance '
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed?


Parameters
----------
instance : bool or {'existing'}, optional, default=True
Copy link
Member

Choose a reason for hiding this comment

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

See https://numpydoc.readthedocs.io/en/latest/format.html#parameters

Suggested change
instance : bool or {'existing'}, optional, default=True
instance : bool or {'existing'}, default=True

Comment on lines +5193 to +5195
If a keyword does not match a defined parameter, it raises a TypeError:

>>> obj = MyClass(nonexistent_param=42) # TypeError: MyClass.__init__() got an unexpected keyword argument 'nonexistent_param'
Copy link
Member

Choose a reason for hiding this comment

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

No need to document this imo, this is standard Python.

**Default Naming**

>>> obj.name
'MyClass12345' # Default name: class name + unique identifier.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'MyClass12345' # Default name: class name + unique identifier.
'MyClass00001' # Default name: class name + unique identifier.


Parameters
----------
**params : dict
Copy link
Member

Choose a reason for hiding this comment

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

See https://numpydoc.readthedocs.io/en/latest/format.html#parameters

Suggested change
**params : dict
**params

>>> obj.my_number = 7
Changed my_number from 5 to 7

`watch` is the most low level, reactive api. For most use cases we recommend
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure watch is what we call a "reactive" API. Also I think it's usually written API or APIs, not api or apis.

@@ -4383,51 +5081,125 @@ def __setstate__(self, state):

class Parameterized(metaclass=ParameterizedMetaclass):
"""
Copy link
Member

Choose a reason for hiding this comment

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

The Numpy docstring guide seems to say that you should document __init__ in the class docstring (here), see https://numpydoc.readthedocs.io/en/latest/format.html#class-docstring. That's what Pandas does for example https://github.com/pandas-dev/pandas/blob/b8a4691647a8850d681409c5dd35a12726cd94a1/pandas/core/frame.py#L505.
So the Parameters section. you added to __init__ should be moved to the class docstring. Apparently you could also add an Attributes section to document name and param.

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.

3 participants