-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
bcc5d03
to
b7fcf69
Compare
Cycling to rebuild docs, now that upstream has been updated |
There was a problem hiding this 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.
galleries/examples/statistics/multiple_histograms_side_by_side.py
Outdated
Show resolved
Hide resolved
Cleaned this one up, thanks! |
The build is broken because of #28820, but fixing that, it's really broken because |
how come? (Last I checked, we don't do any validation on tags) |
I'd prefer that we don't use the tagging mechanism for internal suff. Making internal labels public is confusing, OTOH packing them behind It would work, if |
That should if you put it behind a new tag directive |
Well, I feel uncomfortable, with this duct-tape coding of internal tags via |
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.
Thanks for the heads up. |
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. |
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. |
Is it sufficient to create a collection issue for "example rework"? Instead of adding a
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. |
These are tags that only show up in the dev version, which we generally don't want users using
Or request changes on the PR and have a discussion there?
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. |
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. |
removed the internal tags so this can get merged |
# plot-type: speciality, plot-type: scatter, component: ellipse, component: patch, | ||
# domain: statistics, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter yelling
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 #
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem 😁
There was a problem hiding this comment.
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>
galleries/examples/statistics/multiple_histograms_side_by_side.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
…569-on-v3.9.x Backport PR #27569 on branch v3.9.x (DOC: initial tags for statistics section of gallery)
…569-on-v3.9.2-doc Backport PR #27569 on branch v3.9.2-doc (DOC: initial tags for statistics section of gallery)
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