Skip to content

DOC: initial tags for statistics section of gallery #27569

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 2 commits into from
Oct 28, 2024

Conversation

story645
Copy link
Member

@story645 story645 commented Dec 26, 2023

PR summary

Batch of tags for the stats functions, started by @ktnharris22 in #27235

I mostly didn't add to the tags except to flag stuff that's veering into tutorial or might belong in plot-types. I put/propose stashing internal-tags in an ifconfig b/c I think they can be useful on dev as a flag of what to work on and confusing/clutter on any of the stable versions.

PR checklist

@story645 story645 added status: upstream fix required Documentation: tags tagging examples + proposing new tags labels Dec 26, 2023
@story645 story645 added this to the v3.9.0 milestone Dec 26, 2023
@story645 story645 force-pushed the tag-stats branch 2 times, most recently from bcc5d03 to b7fcf69 Compare December 27, 2023 00:23
@QuLogic QuLogic modified the milestones: v3.9.0, v3.9.1 May 10, 2024
@QuLogic QuLogic modified the milestones: v3.9.1, v3.9.2, v3.9-doc Jun 26, 2024
@ksunden
Copy link
Member

ksunden commented Jul 13, 2024

Cycling to rebuild docs, now that upstream has been updated

@ksunden ksunden closed this Jul 13, 2024
@ksunden ksunden reopened this Jul 13, 2024
@github-actions github-actions bot added the Documentation: examples files in galleries/examples label Jul 13, 2024
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

This also needs a rebase.

@story645
Copy link
Member Author

Cleaned this one up, thanks!

@QuLogic
Copy link
Member

QuLogic commented Sep 14, 2024

The build is broken because of #28820, but fixing that, it's really broken because internal: too-much is not a valid tag.

@story645
Copy link
Member Author

internal: too-much is not a valid tag.

how come? (Last I checked, we don't do any validation on tags)

@timhoffm
Copy link
Member

I'd prefer that we don't use the tagging mechanism for internal suff. Making internal labels public is confusing, OTOH packing them behind .. ifconfig is extra effort (and doesn't work if you want to have public tags at the same time).

It would work, if sphinx-tags had a configuration to exclude certain tags. But that does not exist right now.

@story645
Copy link
Member Author

and doesn't work if you want to have public tags at the same time

That should if you put it behind a new tag directive

@timhoffm
Copy link
Member

Well, I feel uncomfortable, with this duct-tape coding of internal tags via .. ifconfig. It's cluttered and difficult to write for tag authors. But it technically, works and could be refactored later. So I won't block, but please find somebody else to approve.

@story645
Copy link
Member Author

It's cluttered and difficult to write for tag authors.

I agree, I just don't want to lose this information b/c right now I don't have the bandwidth to figure out how to get it working on the sphinx-tag side.

So I won't block, but please find somebody else to approve.

Thanks for the heads up.

@jklymak
Copy link
Member

jklymak commented Sep 17, 2024

Problems with examples should be issues for discussion, not tags.

@story645
Copy link
Member Author

Problems with examples should be issues for discussion, not tags.

Why not both? The idea here is use tags to get a holistic overview and also flag those examples in the dev version. I think that makes it easier to then write an issue, especially if the fix is more holistic.

@jklymak
Copy link
Member

jklymak commented Sep 17, 2024

Because tags are for users to find things in the docs. Issues are for maintainers to track issues.

Basically by tagging an example as "too much" you are issuing a judgement. To remove your judgement I would then have to open a PR to change the tag. That is backwards to how an issue works, where there are no committed changes until a PR is approved.

@timhoffm
Copy link
Member

Is it sufficient to create a collection issue for "example rework"? Instead of adding a internal: needs review tag to the code, add a line in the issue

This is much more lightweight. It can be edited without a PR and review. Also it's possible to add a few words for context to the point. The tag must stand for itself.

The only downside I see is that non-core devs likely don't have the right to edit that issue. But that's not too bad. They can comment on it and we merge the comments in to one top-level list if needed.

@story645
Copy link
Member Author

Because tags are for users to find things in the docs.

These are tags that only show up in the dev version, which we generally don't want users using

To remove your judgement I would then have to open a PR to change the tag.

Or request changes on the PR and have a discussion there?

The only downside I see is that non-core devs likely don't have the right to edit that issue.

That's sorta what motivated having the internal tags in the first place - to give contributors and maintainers a quick way to flag examples that need to be looked at, and using the tags machinery to get a quick list with links of those examples.

The issue version ends up being a top down scan all the examples, while the tag version is "oh, looking through this example for other reasons, it also should get added to the list for review".

For me a compromise would maybe be a pinned issue or template for unclear examples w/ tagging - something where the tracking issue isn't getting buried on page 3.

@jklymak
Copy link
Member

jklymak commented Sep 17, 2024

I'll stand by my stance documentation tags should not be used to organize issues. There are lots of mechanisms to organize and surface issues on GitHub, specifically tags and projects, and we should use those.

@story645
Copy link
Member Author

removed the internal tags so this can get merged

Comment on lines 222 to 223
# plot-type: speciality, plot-type: scatter, component: ellipse, component: patch,
# domain: statistics,
Copy link
Member

@timhoffm timhoffm Oct 28, 2024

Choose a reason for hiding this comment

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

We have discussed this somewhere, but can't find the PR right now: I believe we wanted to use a standard formatting for better readability, and this can be

  • one line for short lists, e.g.
    .. tags:: plot-type: speciality, domain: statistics
    
  • one tag per line for longer lists
    Suggested change
    # plot-type: speciality, plot-type: scatter, component: ellipse, component: patch,
    # domain: statistics,
    # plot-type: speciality
    # plot-type: scatter
    # component: ellipse
    # component: patch
    # domain: statistics

From a quick check, existing tags seem indeed to conform to this formatting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah but this didn't need to be line wrapped/trigger the PEP 8 test and I thought that was the benchmark for "long"

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah but this didn't need to be line wrapped/trigger the PEP 8 test and I thought that was the benchmark for "long"

Copy link
Member

Choose a reason for hiding this comment

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

My definition of "long" is "does not fit on a single line". What's your cut-off? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

The linter yelling

Copy link
Member

Choose a reason for hiding this comment

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

But you started a second line with # domain: statistics,. To extrapolate, I could add more tags there, and when that's full, start a third line and continue.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♀️ sorry for being stupidly stubborn, I thought they were all on one line and didn't catch the #

Copy link
Member

Choose a reason for hiding this comment

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

No problem 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, just pushed up all the changes.

Co-authored-by: katie <katie@unrefugees.org>

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@timhoffm timhoffm merged commit d7ce220 into matplotlib:main Oct 28, 2024
22 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 28, 2024
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 28, 2024
@story645 story645 deleted the tag-stats branch October 28, 2024 23:42
timhoffm added a commit that referenced this pull request Oct 29, 2024
…569-on-v3.9.x

Backport PR #27569 on branch v3.9.x (DOC: initial tags for statistics section of gallery)
greglucas added a commit that referenced this pull request Oct 29, 2024
…569-on-v3.9.2-doc

Backport PR #27569 on branch v3.9.2-doc (DOC: initial tags for statistics section of gallery)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: examples files in galleries/examples Documentation: tags tagging examples + proposing new tags
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants