Skip to content

Proof of concept for adding kwdoc content to properties using a decorator #22699

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

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Mar 24, 2022

PR Summary

As discussed in today's dev call. The effect is visible in the Axes.text docstring:

grafik

Performance prognosis

I slightly underestimated the number of artist properties we have. Instead of a few hundred, we have 4119 artist properties. 😲. Anyway, using a string given via the decorator should be significantly faster than scraping the docstring for such info. I'm 99.9% certain we won't have longer import times due to the decorator mechanism.

Feedback is welcome before I start to roll this out massively.

@timhoffm timhoffm marked this pull request as draft March 24, 2022 23:29
@oscargus
Copy link
Member

I think this looks great! Can also be used to provide information for methods with inherited doc-strings (if that cannot be solved in some other way, e.g., clip_on for Text).

@timhoffm timhoffm force-pushed the artist-property-decorator branch from 2795d83 to 3cfb209 Compare March 31, 2022 18:29
@timhoffm
Copy link
Member Author

Suggested way forward:

  1. Support parameterless @_docstring.kwdoc(), which uses the existing logic from ArtistInspector.get_valid_values to extract the values from the docstring. This saves us duplication of the values documentation if setters and the kwdoc are the same.
  2. Flag all artist setters with this decorator. For simplicity start with the parameterless version everywhere.
  3. Simplify the logic in ArtistInspector.get_setters to look for set_* methods with a _kwdoc attribute. (As an in-between check, make sure that the current logic and the _kwdoc logic match the same methods (i.e. we haven't forgotten to flag any setters).
  4. Once the mechanism is in place, we can start adding more targeted kwdocs. (Can be a separate PR and doesn't have to be all at once).

@timhoffm
Copy link
Member Author

I hold this off for now, because I think it's worth to establish explicit declaration of artist properties and make the kwdoc a part of that. See #22749 for the way forward.

@timhoffm timhoffm force-pushed the artist-property-decorator branch from 3cfb209 to ea9f904 Compare May 21, 2022 21:59
@timhoffm timhoffm marked this pull request as ready for review May 21, 2022 22:23
@timhoffm
Copy link
Member Author

I've put #22749 on hold and am back here. Let's take this step by step: Here is the first step that allows to explicitly set content for kwdoc entries.

Side note: IMHO it's not yet clear whether we'll keep the decorator only for kwdoc in the long run or whether it will grow additional functionality. But that's ok, we can always refactor and rename later because this is private API.

@timhoffm timhoffm added this to the v3.6.0 milestone May 21, 2022
@timhoffm timhoffm force-pushed the artist-property-decorator branch from ea9f904 to 8ab74a7 Compare May 26, 2022 15:54
@tacaswell
Copy link
Member

This seem reasonable to me and will merge the full rollout 👍🏻

@oscargus
Copy link
Member

oscargus commented Jun 2, 2022

I guess one may even consider adding a simpler version just containing the rcparam? Like @_docstring.rcparam("text.parse_math'). If the current parameter description does not contain the "or None"-part it will basically be

current_type + f", default: :rc:`{rcparam}`"

(I guess that the rcparam name is quite related, but maybe not related enough to automatically add that?)

@timhoffm
Copy link
Member Author

timhoffm commented Jun 2, 2022

I guess one may even consider adding a simpler version just containing the rcparam? Like @_docstring.rcparam("text.parse_math').

That depends on the definition of "simple" 😄. I'd argue:

Taking the given string and using it as the kwarg doc entry is pretty simple. Parsing the docstring (maybe stripping None (?)) and putting this plus the given string into a template string is more complex.
Also the former is more explicit.

On a general note: There's a strong believe among developers that duplication is a bad thing and to be avoided at all costs. That's not true per-se. Duplication is only harmful if it makes your code cluttered and thus less readable, or if syncronization becomes an issue - either the risk of getting out of sync or the effort need for changing all places. But synchronization is not an issue if you likely don't change in the future. On the other hand, deduplication always comes at the cost of indirection (how is this description generated?) and stronger coupling (are we sure we always want current_type + f", default: :rc:`{rcparam}`"?) It's always a trade-off; here, I'd claim that clutter and synchronization are not an issue, indirection and coupling are more pressing concerns to me.

(I guess that the rcparam name is quite related, but maybe not related enough to automatically add that?)

I'm pretty sure it's not related enough.

@oscargus
Copy link
Member

oscargus commented Jun 3, 2022

Maybe I wasn't completely clear. I see this as a complement to meet the "yet another thing for new contributors to consider"-argument. This would then only be used if the doc-string does not contain "or None" (i.e., is suitable to start with). It would also add the benefit of a way to directly connect the rcparams to specific setter/kwargs.

For the latter part, one may even consider adding something like:

def kwdoc(text=None, rcparam=None):

where if text is None, the scraper is used and if rcparam is not None, it is added as in my previous example, either to text or the scraped text.

Maybe a bit more time consuming when generating the docs (which indeed is an aspect as well), but will enable to add things in a more flexible manner and can be used for extended things in the future.

@@ -3,6 +3,28 @@
from . import _api


def kwdoc(text):
Copy link
Member

Choose a reason for hiding this comment

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

This name and description is still unclear to me, even after you explained it to me. I think the name should be more descriptive, and the docstring here more explicit about what problem this solves.

Copy link
Member Author

@timhoffm timhoffm Jun 3, 2022

Choose a reason for hiding this comment

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

The name was taken over from the function that generates uses this information to generate the docstring we insert for the kwargs:

def kwdoc(artist):

But I agree, it's a bit mysterious. Would this be better?

def kwarg_doc(text):
    Decorator for defining the documentation used when artist properties are
    passed through as keyword arguments.

    See e.g. the `**kwargs` section in `.Axes.text`.

Copy link
Member

Choose a reason for hiding this comment

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

I still think the above would be more clear

Copy link
Member Author

Choose a reason for hiding this comment

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

Chaned to kwarg_doc.

@timhoffm
Copy link
Member Author

timhoffm commented Jun 3, 2022

def kwdoc(text=None, rcparam=None):

is basically a superset of the API defined here. We can add the rcparam as a shortcut if that's a common enough usecase; which it likely is, but I like to let such API discover itself and not try to design it upfront. I.e. start simple, an when a pattern emerges on use, extract. That makes sure we get exactly what we need. (There's a good talk by Raymond Hettinger propagating this approach).

@QuLogic QuLogic modified the milestones: v3.6.0, v3.7.0 Aug 24, 2022
@tacaswell tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 16, 2022
@jklymak jklymak marked this pull request as draft January 26, 2023 02:20
@jklymak
Copy link
Member

jklymak commented Jan 26, 2023

Does @timhoffm or someone else @oscargus want to move this forward? Moved to draft, but feel free to move back if ready for review...

@timhoffm timhoffm marked this pull request as ready for review January 30, 2023 13:01
@timhoffm
Copy link
Member Author

timhoffm commented Jan 30, 2023

IMHO this is ready as a minimal version of the feature. The usage is demonstrated in one example. Systematic application should be a follow-up PR, After the fundamental concept is approved here.

Extensions can be added later when needed. This is internal API, so we can always refine or reconsider.

@timhoffm timhoffm force-pushed the artist-property-decorator branch 3 times, most recently from 241c20e to ee602b3 Compare February 2, 2023 23:54
Comment on lines 11 to 21
See e.g. the `**kwargs` section in `.Axes.text`.

The given text is stored in a privat variable ``_kwarg_doc`` on the method.
It is used for generating the kwdoc list for artists, which we
auto-generate through docstring interpolation, e.g. via
``%(Line2D:kwdoc)s``.

The text should contain the supported types as well as the default value
if applicable, e.g.:

@_docstring.kwarg_doc("bool, default: :rc:`text.usetex`")
def set_usetex(self, usetex):
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 finding this a little hard to follow and wondering if it'd be clearer if this was streamlined to one use case - so like an example of a method that uses line2D kwargs and then a code example that applies this decorator using a line2d kwarg.

@oscargus oscargus force-pushed the artist-property-decorator branch from d8a91ba to d47f986 Compare April 7, 2023 13:19
@oscargus
Copy link
Member

oscargus commented Apr 7, 2023

I also rebased and used @jklymak's documentation, with some modifications.

@oscargus
Copy link
Member

So there is a "new" error in that **kwargs cannot be found, but easily fixed if we do not need a link there...

Anyway, it would be of interest to make some progress on this and decide on which approach to proceed with.

@jklymak
Copy link
Member

jklymak commented Jul 13, 2023

@oscargus I'm happy to approve this if its ready to go?

@ksunden
Copy link
Member

ksunden commented Jul 16, 2023

Is this a proof of concept that should not be merged (as the title suggests) or ready to merge (as the status: needs review label suggests)?

I've mostly been dropping this down my review list because of the title.

@oscargus
Copy link
Member

I think something like this should be really nice to have. Maybe we should discuss it (once more?) at a dev call before making a decision on which approach.

On the other hand, it is not a big deal to merge this and then decide that we would like a slightly different approach. (The big effort is adding decorators...)

@jklymak
Copy link
Member

jklymak commented Dec 9, 2023

Does this still need discussion?

@jklymak jklymak added the status: needs comment/discussion needs consensus on next step label Dec 9, 2023
@timhoffm timhoffm force-pushed the artist-property-decorator branch from d47f986 to a7a0a67 Compare December 16, 2023 09:12
@timhoffm
Copy link
Member Author

From my perspective, this is ready to go.


See e.g. the ``**kwargs`` section in `.Axes.text`.

The given text is stored in a privat variable ``_kwarg_doc`` on the method.
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
The given text is stored in a privat variable ``_kwarg_doc`` on the method.
The given text is stored in a private attribute ``_kwarg_doc`` on the method.

Copy link
Member

Choose a reason for hiding this comment

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

Should this also explicitly say it is for use on the Artist property setter, not just any random method.

Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

On the one hand I don't want to nitpick the docstring to death, on the other I wonder if clarity could be improved by not aiming for conciseness.

Comment on lines 14 to 16
It is used for generating the kwdoc list for artists, which we
auto-generate through docstring interpolation, e.g. via
``%(Line2D:kwdoc)s``.
Copy link
Member

Choose a reason for hiding this comment

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

The phrasing of this implies that this is already implemented via interpolation

Comment on lines 8 to 9
Decorator for defining the documentation used when artist properties are
passed through as keyword arguments.
Copy link
Member

Choose a reason for hiding this comment

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

Having a little bit of trouble following this - is there a more direct way of saying what this decorator does?
My reading is that the decorator stores documentation for the Artist property and the documentation stored on the decorator is what populates interpolated fields for that Artist?

@timhoffm timhoffm force-pushed the artist-property-decorator branch from 745992f to aaf53c9 Compare December 21, 2023 10:05
@timhoffm
Copy link
Member Author

I've rewritten the docstring to hopefully better explain what this does.

@tacaswell tacaswell merged commit 075c5bc into matplotlib:main Jan 11, 2024
story645 added a commit to story645/matplotlib that referenced this pull request Jan 11, 2024
Got merged with small typo in docstring (surprised it wasn't caught by the pre-commit :/) so fixed that. Also added a comma b/c phrase seemed parenthetical.
story645 added a commit to story645/matplotlib that referenced this pull request Jan 11, 2024
Got merged with small typo in docstring (surprised it wasn't caught by the pre-commit :/) so fixed that. Also added a comma b/c phrase seemed parenthetical.
@timhoffm timhoffm deleted the artist-property-decorator branch January 11, 2024 21:06
ksunden added a commit that referenced this pull request Jan 11, 2024
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.

7 participants