Boxplot fix median line extending past box boundaries #19409 #26462
Conversation
Other changes: - Update `.gitignore` to ignore `venv/` in commit - Fixed a small typo in `bxp()` - Added a new test for the median line (from PR matplotlib#23335)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
Hi @kidkoder432 - while we wait for a proper review, would you mind taking a look at the Linting failure here? https://github.com/matplotlib/matplotlib/actions/runs/5780696475/job/15664707497?pr=26462 It looks like you are missing a blank line in your test. |
There was a problem hiding this comment.
Thank you for your work on this @kidkoder432, but I do not think this is currently the right change.
rcomer
added
status: needs revision
PR: bugfix
|
Oh okay, thanks! (This is my first contribution to matplotlib so I
appreciate the feedback :))
…On Tue, Aug 8, 2023 at 12:01 AM Ruth Comer ***@***.***> wrote:
***@***.**** commented on this pull request.
Thank you for your work on this @kidkoder432
<https://github.com/kidkoder432>, but I do not think this is currently
the right change.
------------------------------
In lib/matplotlib/axes/_axes.py
<#26462 (comment)>
:
> @@ -4095,7 +4095,7 @@ def bxp(self, bxpstats, positions=None, widths=None, vert=True,
capwidths : float or array-like, default: None
Either a scalar or a vector and sets the width of each cap.
- The default is ``0.5*(with of the box)``, see *widths*.
+ The default is ``0.5*(width of the box)``, see *widths*.
Nice spot!
------------------------------
In lib/matplotlib/lines.py
<#26462 (comment)>
:
> @@ -297,7 +297,7 @@ def __init__(self, xdata, ydata, *,
markerfacecoloralt='none',
fillstyle=None,
antialiased=None,
- dash_capstyle=None,
+ dash_capstyle="butt",
I think you intended to change *solid_capstyle* rather than
*dash_capstyle*, but I do not think we want to do this here. This will
define the default capstyle for lines used all over the library, and will
override what the user has set in their rcParams (see the logic below
with if solid_capstyle is None:, etc.)
For the current issue, I think we only want to change the capstyle used by
default within boxplot for the median line. As noted in #19409
<#19409>, you can get at
that in the medianprops dictionary.
—
Reply to this email directly, view it on GitHub
<#26462 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOYIALWGZ6KQXTFQWGJIMH3XUHP5BANCNFSM6AAAAAA3GLM3QA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
This change has caused some test failures. @kidkoder432 are you confident to investigate those yourself? You can see the full output from the tests by clicking the "Details" link by one of the checks at the bottom of this page. You can also run the tests yourself locally: |
Sure, I'll take a look and let you know any concerns. |
|
Edit: I fixed the Okay, so I looked at the failing tests and noticed that the expected images were all using a capstyle of
I'm not sure how to move forward with this. |
Sometimes the `medianprops` dict was not provided so the previous code was failing to assign a key to it. So a check was added to either modify an exsisting dict or create a new one.
|
OK, if the user specifies a capstyle then we should assume they are happy with their choice and leave it. We should only set it to “butt” if the user didn’t make a choice. I think the dictionary |
Good idea. |
|
Ah OK, I thought you meant that some tests were specifying “projecting” as the capstyle. Instead they are just leaving it to default and this change switches the default from “projecting” to “butt”. So the tests are failing because of a genuine (desired) change in the output, which means you will need to replace the reference images. We have the triage_tests tool to help with that. |
I used the triage tests tool to set the test benchmark images to those with a capstyle of `butt`. I also modieifed the behaviror for setting the capstyle for median lines to obey the user's preference instead of overriding it.
|
Thanks for the update @kidkoder432. It looks like you still have a couple of image test failures for pdf files. Do you see those failures locally? If not you might be missing a dependency - most likely ghostscript. |
|
i don't see them locally; i probably do need to install ghostscript.
How would I resolve the PDF test failures?
…On Wed, Aug 16, 2023, 12:00 PM Ruth Comer ***@***.***> wrote:
Thanks for the update @kidkoder432 <https://github.com/kidkoder432>. It
looks like you still have a couple of image test failures for pdf files. Do
you see those failures locally? If not you might be missing a dependency -
most likely ghostscript.
https://matplotlib.org/devdocs/devel/dependencies.html#dependencies-for-testing-matplotlib
—
Reply to this email directly, view it on GitHub
<#26462 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOYIALX7ALTX6FGE4YKIQ33XVUKGTANCNFSM6AAAAAA3GLM3QA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I think the |
|
Got it, thank you!
…On Wed, Aug 16, 2023, 12:10 PM Ruth Comer ***@***.***> wrote:
I think the triage_tests script should handle pdf as well as png.
—
Reply to this email directly, view it on GitHub
<#26462 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOYIALQJRPPJJSWYAPEUTYLXVULKTANCNFSM6AAAAAA3GLM3QA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I'm having trouble getting pytest to recognize that I have Ghostscript installed. are there any resources for this on the matplotlib docs? i used the installer to install Ghostscript 10.01.2. |
|
Matplotlib only interacts with ghostscript via subprocess calls. Can you call If it is not in your PATH, it will not be found, so make sure it is installed in a place that it can be executed. |
|
I use VSCode for development. If I run gswin64 from a regular powershell
terminal, it launches a command window to type gs commands with (gs is in
system PATH). However, this command does not work in VSCode, and pdf tests
are skipped by pytest.
How do I resolve this so i can continue development? Thanks.
…On Fri, Aug 18, 2023 at 12:17 AM Kyle Sunden ***@***.***> wrote:
Matplotlib only interacts with ghostscript via subprocess calls. Can you
call gs as a CLI program (e.g. gs --help) (on windows it may also be
called gswin64c, not really sure what is fully going on there...)
If it is not in your PATH, it will not be found, so make sure it is
installed in a place that it can be executed.
—
Reply to this email directly, view it on GitHub
<#26462 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOYIALUH646NVJQ3JGQTYZLXV4JHRANCNFSM6AAAAAA3GLM3QA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Never mind, i got it to work.
…On Fri, Aug 18, 2023, 6:22 PM prajwal agrawal ***@***.***> wrote:
I use VSCode for development. If I run gswin64 from a regular powershell
terminal, it launches a command window to type gs commands with (gs is in
system PATH). However, this command does not work in VSCode, and pdf tests
are skipped by pytest.
How do I resolve this so i can continue development? Thanks.
On Fri, Aug 18, 2023 at 12:17 AM Kyle Sunden ***@***.***>
wrote:
> Matplotlib only interacts with ghostscript via subprocess calls. Can you
> call gs as a CLI program (e.g. gs --help) (on windows it may also be
> called gswin64c, not really sure what is fully going on there...)
>
> If it is not in your PATH, it will not be found, so make sure it is
> installed in a place that it can be executed.
>
> —
> Reply to this email directly, view it on GitHub
> <#26462 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AOYIALUH646NVJQ3JGQTYZLXV4JHRANCNFSM6AAAAAA3GLM3QA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
The boxplot PDFs were using the wrong capstyle, so I modified the references to use the butt captyle.
|
Hi, for some reason it says that CodeCov tests failed in the workflow logs. However, when I visit the website, its says that codecov tests all passed. Can someone please take a look at this? |
|
I wouldn’t worry about that codecov failure. We currently use codecov for information only. In this case, codecov says that 100% of your change is covered, which I think is the important thing. The coverage reductions are all “indirect changes” - I haven’t really got my head around how those are caused myself. |
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
|
Yes, it makes sense. Thank you for the explanation!
…On Sat, Aug 19, 2023 at 10:40 AM Ruth Comer ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/matplotlib/axes/_axes.py
<#26462 (comment)>
:
> + if medianprops is None: # Clamp median line to edge of box
+ medianprops = {"solid_capstyle": "butt"}
+ else:
+ medianprops.setdefault("solid_capstyle", "butt")
Good question.
Suppose a user has a dictionary of properties that they re-use for
multiple different plot types in their script. If that dictionary gets
added to when they pass it to boxplot, then it could affect other plots
that are created further down the script. That would be unexpected (and
therefore undesirable) behaviour.
Does that make sense? You are right that setdefault won’t change the
dictionary if it already has that key.
—
Reply to this email directly, view it on GitHub
<#26462 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOYIALRSMA26DNRGEXL4GNDXWD3CHANCNFSM6AAAAAA3GLM3QA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
This looks good to me now. For code changes, we require approvals from two reviewers so this can’t be merged just yet. Thank you for your work on it @kidkoder432!
There was a problem hiding this comment.
Question: Does anybody understand why the box borders are sometimes shifted slightly?
Comment: The original issue also discussed bringing the line below the box using zorder. This is not discussed in this PR, While conceptually desireable, this is not so easy to fix: The box can be filled and then face and edge are drawn with the same zorder. This means, one cannot draw mean/median lines below the box edge but above the box face (at least not without complicating the draw logic).
I'm therefore inclinded to accept the PR without addressing zorder, but would like to have at least one second thought on this.
| @@ -4147,6 +4147,11 @@ def bxp(self, bxpstats, positions=None, widths=None, vert=True, | |||
| -------- | |||
| .. plot:: gallery/statistics/bxp.py | |||
| """ | |||
| # Clamp median line to edge of box by default. | |||
| medianprops = { | |||
| "solid_capstyle": "butt", | |||
There was a problem hiding this comment.
I think we should unversally "solid_capstyle" and "dashed_capstyle" to "butt" for both medianprops and meanprops. We want the lines to end within the box for both independent of the global capstyle settings.
Not sure why either... From what you explained about the draw logic it would likely be much easier to stay with the current implementation. |
Do you have an example? When I looked at the test images that have changed here, all the diffs seemed to just show the median line getting shorter.
I came to same conclusion based on @oscargus' comment on the previous PR: #23335 (comment) |
Can't find anymore. Either I was wrong or this was somehow fixed through the recent updates. |
|
Since this is a bugfix, I recon that it can still go into 3.8.0 (on the basis that we do not have release candidates for any bugfix releases, there's no difference between deferring to 3.8.1 and pushing this into 3.8.0 even after 3.8.0rc1). |
|
Thanks @kidkoder432, and congratulations on your first contrbution to matplotlib! We'd happy to see you back! |
|
You're welcome! Thank you for your support!
I'll definitely be back to contribute :)
…On Mon, Aug 21, 2023, 1:28 PM Tim Hoffmann ***@***.***> wrote:
Thanks @kidkoder432 <https://github.com/kidkoder432>, and congratulations
on your first contrbution to matplotlib! We'd happy to see you back!
—
Reply to this email directly, view it on GitHub
<#26462 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOYIALXG77XIXJIN4LCEWILXWPAHDANCNFSM6AAAAAA3GLM3QA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
PR summary
Fixes #19409
The median line on a boxplot would extend past the box when the line width was increased. This PR aims to fix the visual annoyance by setting the solid line capstyle to be the same as all other capstyles. additionally, this change keeps the capstyle of the median lines consistent with other boxplot lines (i.e. mean lines).
This is my second ever PR so be nice please :)
Other changes:
bxp()docstringPR checklist