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
Fillbetween #6560
Fillbetween #6560
Conversation
You targeted the right branch. If merged, this will ship with 2.1 at the earliest. |
""" | ||
Make filled polygons between two horizontal curves. | ||
|
||
Call signature:: | ||
|
||
fill_betweenx(y, x1, x2=0, where=None, **kwargs) | ||
fill_betweenx(y, x1, x2=0, where=None, step=None, **kwargs) |
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.
can you also add interpolate
here?
Can you add an entry in |
What is the problem here? I can't see the indicated errors.
|
That is odd that it is reporting lines outside of where you touched. This may be due to a bad merge somewhere else? |
|
There is once again a flake failure, in a line that is not part of the diff - 4908 (and I have already configured the editor to remove trailing spaces on write). There is an unrelated test failing as well at the image comparison: What should I do? |
|
||
The ``interpolate`` parameter now exists for the method :func:`fill_betweenx`. This allows | ||
a user to interpolate the data and fill the areas in the crossover points, similarly to :func:`fill_between`. | ||
:: |
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 ::
indicates there will be a code block following; it should be removed (or an example added.)
That comes and goes; you can ignore it. |
I will implement the tests for |
@tacaswell I was just wondering, since this is such a trivial change, why not merge this before 2.1? Would it make a difference if the test coverage increased? |
This is a new feature and 2.1 is the next release with planned new features. The 1.5.x series is only getting critical (as in fixing regressions / does not compile / show stopper) bug fixes, and the 2.x series is only getting changes needed to get the style updates out the door. This seems to need a rebase. |
ping @rmlopes can you rebase this? |
@tacaswell I rebased by squashing every commit into the first, then forced the push as the history is incompatible. Looks like this may not have been the correct approach though. |
You now need to rebase that single commit onto current master and force-push again. |
Should be ok now. |
Something went wrong here, htere should not be this many commits. See http://matplotlib.org/devdocs/devel/gitwash/development_workflow.html?highlight=rebase#rebasing-a-pull-request-pr |
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.
Overall, this looks good.
I've added a comment, but I think this PR has been going on for so long that we should merge it and do the maintenance in another PR.
Y = np.zeros((2 * N + 2, 2), float) | ||
Y = np.zeros((2 * N + 2, 2), np.float) | ||
if interpolate: | ||
def get_interp_point(ind): |
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.
Is this the exact same function as in fill_between
?
If so, I think it should be factored out and moved to the cbook module as a private function.
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 so. I thought about that but also about using some interpolation function maintained and alerady available in scipy (would this be a bad idea perhaps?). I'll have a look at it as soon as I can.
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.
scipy isn't a dependency, so using their functions isn't an option unless you copy paste them in a module of ours.
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 two methods differ on the axes of interpolation only. Could be a single function indeed. I can make this in a new branch but I need to know where to add the interpolate function.
This solution is actually very slow for my use case since I am displaying a few thousand seismic traces, so I would like to keep working on this.
Can you amend the commit and edit the commit message to remove the duplicate information and the comments from git? |
I did a rebase squashing 2 commits but somehow ended up with three commits. |
Thanks @rmlopes ! I'll merge this as soon as the tests have passed. |
Interpolation in fill_betweenx | ||
------------------------------ | ||
|
||
The ``interpolate`` parameter now exists for the method :func:`fill_betweenx`. |
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.
It's a method on a class, not a function, so will this cross-reference work?
""" | ||
Make filled polygons between two horizontal curves. | ||
|
||
Call signature:: |
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.
Please don't add the call signature back; it is redundant.
…nabled through keyword argument interpolate.
On 2016/11/01 10:10 AM, Rui Lopes wrote:
Yes, there should be some scope for vectorization here. At the very |
I'll merge this, but @rmlopes feel free to continue working on this and open a new PR! |
This backports matplotlib/matplotlib#6560 so that shading things like CAPE/CIN now properly fill up to the cross-over point.
As discussed in issue #6543. Added the interpolate argument just before the
**kwargs
.I think I made a mistake, I wanted to target the 1.5.x. I'll be waiting for some input. :)
Should I edit the changelog as well?