Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add Axes.fill_disjoint to make invert fill_between easier#24742
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
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.
Thank you for this! I just left some minor comments for now.
The imagetest.png should be removed as it is not used(?).
This is a good start (and the code quality is high, with good documentation!), but expect that there are things to be discussed.
For your info: we are in the final stages of feature freezing 3.7, so it may take a while before you get proper feedback (or it may not).
This could also do with a release note. Although in some sense it is OK to wait with that until the reviews start to converge, it can also be a good option to highlight the feature. For example, showing off different options and capabilities. See, e.g.,https://github.com/matplotlib/matplotlib/blob/main/doc/users/next_whats_new/ellipse_annotation.rst what it may look like. (In your case I would go with a subplot with few different plots.)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/patches.py Outdated
| @@ -1066,6 +1066,123 @@ def set_data(self, values=None, edges=None, baseline=None): | |||
| self.stale = True | |||
| class Lines(Patch): | |||
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.
I think this needs a better name.BoundedSemiplane?
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/patches.py Outdated
| """ | ||
| *xy* is a numpy array with shape Nx2. | ||
| If *direction* is *True*, the patch is lower bounded by the polyline. |
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.
I wonder if there is a better name fordirection?lowerbounded?lower?lower_bounded?
lib/matplotlib/tests/test_axes.py Outdated
| @@ -45,6 +45,30 @@ | |||
| # different baseline images to prevent race conditions when pytest runs | |||
| # the tests with multiple threads. | |||
| @image_comparison(['fill_disjoint'], remove_text=False) | |||
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.
| @image_comparison(['fill_disjoint'],remove_text=False) | |
| @image_comparison(['fill_disjoint.png'],remove_text=False,style='mpl20') |
lib/matplotlib/tests/test_axes.py Outdated
| fig, ax = plt.subplots() | ||
| plt.plot(xRng, [f1(x) for x in xRng], 'b-') | ||
| plt.plot(xRng, [f2(x) for x in xRng], 'r-') | ||
| # Uncomment below line to test other configs |
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.
I think this should be removed. Or simply just add another test.
…baseline generation code
Thank you so much@oscargus for these helpful suggestions. We just included them in our most recent commit. |
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
…ean to {'top', 'bottom'}This is failing all the tests. Can you include a description in the PR of what this does with code? Maybe also argue for the need for the extra complexity - it's not clear to me we should add all this. Finally, the new tests probably don't need to be in all the different file types? |
I'll move to draft, but feel free to move back when ready to move forward (if desired). Thanks! |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
This PRfixes#23768, which asks for an easy solution to invert the fill between 2 lines. In this PR, we wrote a new patch called Lines which bounds the y axis and fixes the unbounded limits problem discussed in the issue. We also wrote the function fill_disjoint which fills in the area excluding the region in between the lines bounded by (x, y1) and (x, y2), which are inputted into the function. Thus, these changes allow users to invert fill of simple shapes composed of straight lines.
PR Checklist
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst