Fillbetween by rmlopes · Pull Request #6560 · matplotlib/matplotlib

rmlopes · 2016-06-08T17:37:53Z

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?

tacaswell · 2016-06-08T18:18:48Z

You targeted the right branch. If merged, this will ship with 2.1 at the earliest.

tacaswell · 2016-06-08T18:19:32Z

lib/matplotlib/axes/_axes.py

        Call signature::

-          fill_betweenx(y, x1, x2=0, where=None, **kwargs)
+          fill_betweenx(y, x1, x2=0, where=None, step=None, **kwargs)


can you also add interpolate here?

tacaswell · 2016-06-08T18:19:58Z

Can you add an entry in docs/user/whats_new (the README in that folder has instructions).

rmlopes · 2016-06-08T22:42:20Z

What is the problem here? I can't see the indicated errors.

AssertionError: 2 != 0 : Found code syntax errors (and warnings):
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:4806:80: E501 line too long (88 > 79 characters)
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:4907:60: W291 trailing whitespace

tacaswell · 2016-06-08T23:36:02Z

That is odd that it is reporting lines outside of where you touched. This may be due to a bad merge somewhere else?

QuLogic · 2016-06-08T23:43:29Z

master probably has some deleted lines so the merge commit that's being tested has different line numbers. However, it's pretty obvious that line 4856 is too long. As for the trailing whitespace, it will be easier if you configure your editor to strip them on save.

rmlopes · 2016-06-09T12:37:41Z

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:
FAIL: matplotlib.tests.test_mathtext.test_mathfont_dejavusans_03.test

What should I do?

QuLogic · 2016-06-09T23:54:11Z

doc/users/whats_new/updated_fill_betweenx.rst

+
+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`.
+::


The :: indicates there will be a code block following; it should be removed (or an example added.)

QuLogic · 2016-06-09T23:55:06Z

There is an unrelated test failing as well at the image comparison:
FAIL: matplotlib.tests.test_mathtext.test_mathfont_dejavusans_03.test

That comes and goes; you can ignore it.

rmlopes · 2016-06-13T11:23:32Z

I will implement the tests for fill_betweenx with and without interpolate because it is missing. I have had some issues with the figures in other libraries (small differences in the fonts to the generated by travis). Is there some kind of procedure (like run the test in travis first, then go to the error message to pick the expected image) in place?

rmlopes · 2016-06-23T13:35:48Z

@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?

tacaswell · 2016-06-23T14:20:02Z

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.

tacaswell · 2016-07-12T05:22:00Z

ping @rmlopes can you rebase this?

rmlopes · 2016-07-13T12:39:30Z

@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.

tacaswell · 2016-07-13T12:42:16Z

You now need to rebase that single commit onto current master and force-push again.

rmlopes · 2016-07-13T13:16:39Z

Should be ok now.

tacaswell · 2016-10-29T01:21:04Z

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

NelleV

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.

NelleV · 2016-10-29T17:05:56Z

lib/matplotlib/axes/_axes.py

-            Y = np.zeros((2 * N + 2, 2), float)
+            Y = np.zeros((2 * N + 2, 2), np.float)
+            if interpolate:
+                def get_interp_point(ind):


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.

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.

scipy isn't a dependency, so using their functions isn't an option unless you copy paste them in a module of ours.

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.

QuLogic · 2016-10-29T17:34:05Z

Can you amend the commit and edit the commit message to remove the duplicate information and the comments from git?

rmlopes · 2016-11-01T17:04:11Z

I did a rebase squashing 2 commits but somehow ended up with three commits.

NelleV · 2016-11-01T17:52:41Z

Thanks @rmlopes ! I'll merge this as soon as the tests have passed.

QuLogic · 2016-11-01T19:27:24Z

doc/users/whats_new/updated_fill_betweenx.rst

+Interpolation in fill_betweenx
+------------------------------
+
+The ``interpolate`` parameter now exists for the method :func:`fill_betweenx`.


It's a method on a class, not a function, so will this cross-reference work?

QuLogic · 2016-11-01T19:27:43Z

lib/matplotlib/axes/_axes.py

        """
        Make filled polygons between two horizontal curves.

+        Call signature::


Please don't add the call signature back; it is redundant.

…nabled through keyword argument interpolate.

efiring · 2016-11-01T20:53:18Z

On 2016/11/01 10:10 AM, Rui Lopes wrote:

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.

Yes, there should be some scope for vectorization here. At the very
least, all necessary interpolation can be done in a single call to
interp for the whole array. I suspect the entire operation can be
vectorized; I don't see any inherent need for loops.

NelleV · 2016-11-05T00:14:01Z

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.

mdboom added the status: needs review label Jun 8, 2016

tacaswell reviewed Jun 8, 2016
View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Jun 8, 2016

QuLogic reviewed Jun 9, 2016
View reviewed changes

tacaswell added status: needs revision and removed status: needs review labels Jul 12, 2016

rmlopes force-pushed the fillbetween branch from 6b4d3ce to 3f38eaf Compare July 13, 2016 12:37

rmlopes force-pushed the fillbetween branch from 3f38eaf to 391198e Compare July 13, 2016 13:15

tacaswell added status: needs review and removed status: needs revision labels Jul 13, 2016

rmlopes force-pushed the fillbetween branch from 4db98a1 to 5a6c779 Compare October 29, 2016 11:49

NelleV reviewed Oct 29, 2016

View reviewed changes

NelleV changed the title ~~Fillbetween~~ [MRG+1] Fillbetween Oct 29, 2016

rmlopes force-pushed the fillbetween branch from 3c75249 to 1ac9e56 Compare November 1, 2016 17:03

QuLogic reviewed Nov 1, 2016

View reviewed changes

added interpolate functionaly to fill_betweenx, as in fill_between, e…

4e9c7e7

…nabled through keyword argument interpolate.

rmlopes force-pushed the fillbetween branch from 1ac9e56 to 4e9c7e7 Compare November 1, 2016 19:58

NelleV merged commit 3ff9fa8 into matplotlib:master Nov 5, 2016

QuLogic changed the title ~~[MRG+1] Fillbetween~~ Fillbetween Nov 5, 2016

QuLogic removed the status: needs review label Nov 24, 2016

dopplershift mentioned this pull request Dec 9, 2016

skew.ax.fill_betweenx shading not shading entire region Unidata/MetPy#69

Closed

Uh oh!

Conversation

rmlopes commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented Jun 8, 2016

Uh oh!

tacaswell Jun 8, 2016

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jun 8, 2016

Uh oh!

rmlopes commented Jun 8, 2016

Uh oh!

tacaswell commented Jun 8, 2016

Uh oh!

QuLogic commented Jun 8, 2016

Uh oh!

rmlopes commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuLogic Jun 9, 2016

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Jun 9, 2016

Uh oh!

rmlopes commented Jun 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmlopes commented Jun 23, 2016

Uh oh!

tacaswell commented Jun 23, 2016

Uh oh!

tacaswell commented Jul 12, 2016

Uh oh!

rmlopes commented Jul 13, 2016

Uh oh!

tacaswell commented Jul 13, 2016

Uh oh!

rmlopes commented Jul 13, 2016

Uh oh!

tacaswell commented Oct 29, 2016

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

NelleV Oct 29, 2016

Choose a reason for hiding this comment

Uh oh!

rmlopes Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

rmlopes Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Oct 29, 2016

Uh oh!

rmlopes commented Nov 1, 2016

Uh oh!

NelleV commented Nov 1, 2016

Uh oh!

QuLogic Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

QuLogic Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

efiring commented Nov 1, 2016

Uh oh!

NelleV commented Nov 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

rmlopes commented Jun 8, 2016 •

edited

Loading

rmlopes commented Jun 9, 2016 •

edited

Loading

rmlopes commented Jun 13, 2016 •

edited

Loading

rmlopes Nov 1, 2016 •

edited

Loading