fix markevery plot option with nans in data #22519

rc · 2022-02-21T10:48:16Z

PR Summary

Hi matplotlib developers,

This PR addresses the following code failure (with matplotlib '3.6.0.dev1701+gb80bc91827', Python 3.8.10):

import numpy as np
import matplotlib.pyplot as plt

x = np.arange(100, dtype=np.float64)
y = x**2
y[50:80] = np.nan

plt.plot(x, y, 'o', markevery=0.1)

plt.show()

where the error is

  File "/home/share/software/packages/matplotlib/lib/matplotlib/backends/backend_qt.py", line 458, in _draw_idle
    self.draw()
  File "/home/share/software/packages/matplotlib/lib/matplotlib/backends/backend_agg.py", line 426, in draw
    self.figure.draw(self.renderer)
  File "/home/share/software/packages/matplotlib/lib/matplotlib/artist.py", line 73, in draw_wrapper
    result = draw(artist, renderer, *args, **kwargs)
  File "/home/share/software/packages/matplotlib/lib/matplotlib/artist.py", line 50, in draw_wrapper
    return draw(artist, renderer)
  File "/home/share/software/packages/matplotlib/lib/matplotlib/figure.py", line 2860, in draw
    mimage._draw_list_compositing_images(
  File "/home/share/software/packages/matplotlib/lib/matplotlib/image.py", line 133, in _draw_list_compositing_images
    a.draw(renderer)
  File "/home/share/software/packages/matplotlib/lib/matplotlib/artist.py", line 50, in draw_wrapper
    return draw(artist, renderer)
  File "/home/share/software/packages/matplotlib/lib/matplotlib/axes/_base.py", line 3105, in draw
    mimage._draw_list_compositing_images(
  File "/home/share/software/packages/matplotlib/lib/matplotlib/image.py", line 133, in _draw_list_compositing_images
    a.draw(renderer)
  File "/home/share/software/packages/matplotlib/lib/matplotlib/artist.py", line 50, in draw_wrapper
    return draw(artist, renderer)
  File "/home/share/software/packages/matplotlib/lib/matplotlib/lines.py", line 819, in draw
    subsampled = _mark_every_path(
  File "/home/share/software/packages/matplotlib/lib/matplotlib/lines.py", line 171, in _mark_every_path
    marker_delta = np.arange(start * scale, delta[-1], step * scale)
ValueError: arange: cannot compute length

I have fixed the issue by replacing the non-finite values by means for each axis - not sure if it is right, but it is simple and works well for my data. Let me know if anything fancier is needed.

Best regards,
r.

PR Checklist

Tests and Styling

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

github-actions

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

oscargus · 2022-02-21T14:57:51Z

(I cannot try out the patch right now. So if you can attach a plot that would help at least me.)

Thanks for this! It is clearly an issue that should be fixed.

Isn't there a risk that markers end up where the value is NaN with this fix? (Probably clearer is '-o' is used, and maybe other nan-locations/markevery-values?) Not sure how one really should deal with it though.

rc · 2022-02-21T17:53:25Z

I have tried a number of functions, such as

import numpy as np
import matplotlib.pyplot as plt

x = -50 + np.arange(100, dtype=np.float64)
y = 10000 + x**5
y[40:95] = np.nan
y[14:20] = np.nan

plt.plot(x, y, '-o', markevery=0.1)
plt.tight_layout()
plt.savefig('nans-marker.png', bbox_inches='tight')
plt.show()

and the results seemed OK (did not manage to get markers outside the line):

Compare with the full curve without the nans:

I guess the logic where to plot something is elsewhere, determined by the original data with the nans. But, of course, take it with the grain of salt - I do not really understand how matplotlib plotting works at this level :)

oscargus · 2022-02-22T09:28:36Z

Thanks for the figures! I've manage to try out the patch a bit now and I agree with your conclusion that it doesn't look like there will markers on NaN. However, it is possible to provoke it to do some "strange" things, but as I mentioned earlier, it may in these cases not be obvious what one really would like to obtain. Still, based on the documentation: " markers will be spaced at approximately equal visual distances along the line; the distance along the line between markers is determined by multiplying the display-coordinate distance of the axes bounding-box diagonal by the value of every." or the demo: "Float arguments allow markers to be spaced at approximately equal distances along the line.", I would not expect it to become like:

or

Maybe an alternative approach would be to compute the "theoretical position" and then remove them if the closest actual data point is NaN?

(On the other hand, the gallery shows some irregular spacing behavior when zoomed in: https://matplotlib.org/stable/gallery/lines_bars_and_markers/markevery_demo.html )

rc · 2022-02-22T10:59:45Z

What is the "theoretical position" when there are nans? I guess the correct approach would be to split the line to consecutive chunks of non-nans and apply the markevery there - but then what if one has data like y = [1, nan, 2, nan, 3, nan]... It seems tricky. BTW. I have tried:

x = -50 + np.arange(100, dtype=np.float64)
y = 10000 + x**5
y[::2] = np.nan

plt.plot(x, y, '-o', markevery=0.07)
plt.plot(x, y, 'x', markevery=(1, 2))

and got

The crosses show the non-nans... It does not look too bad for me, although there are many more markes than there would be in case of the uninterrupted curve.

Then I have tried to fill the nans with an average of the neighbouring non-nans:

-> it seems better, but the code is much more complicated (first try, so it might be possible to simplify):

            aux = np.empty_like(tpath.vertices)
            for ic, vals in enumerate(tpath.vertices.T):
                mask = np.isfinite(vals)
                ii = np.where(mask, np.arange(mask.shape[0]), -1)
                i0, i1 = np.nonzero(ii >= 0)[0][[0, -1]]
                ii[:i0] = i0
                ilo = np.maximum.accumulate(ii)
                ii[~mask] = mask.shape[0] - 1
                ii[i1:] = i1
                ihi = np.minimum.accumulate(ii[::-1])[::-1]
                aux[:, ic] = 0.5 * (vals[ilo] + vals[ihi])

            disp_coords = affine.transform(aux)

rc · 2022-02-22T12:17:30Z

I have tried to polish it a bit, see the current version.

oscargus · 2022-02-22T21:19:41Z

What is the "theoretical position" when there are nans?

You are right. Maybe one can assume some sort of maximum derivative based on the existing data? But it will probably be possible to find edge cases independent of how it is done.

To some extent the most important thing is that it doesn't give an error... (And the behavior I was considering is obtained for an integer markevery, sort of.)

y = [1, nan, 2, nan, 3, nan].

This is indeed a tricky one, especially since any type of line disappears.

I'm a bit too junior when it comes to approving stuff that actually defines a behavior, but I think the new version is probably as good as I can think of. Hopefully someone more experienced can chip in.

rc · 2022-02-23T15:16:38Z

Thanks for the feedback. I have now tried adding a test so that the code coverage passes.

rc · 2022-03-06T16:25:21Z

Ping @matplotlib/developers - the tests pass, not sure if this counts as a new feature for the docs checklist - previously the code just failed.

oscargus · 2022-03-06T18:39:38Z

Thanks and sorry for forgetting about this. I think the fact that it is fixed is great and I think that at the end of the day one can probably always find NaN-patterns that gives non-ideal behavior (especially since one cannot really tell what the ideal behavior is).

So for me it is a go.

rc · 2022-03-06T20:32:41Z

Thanks!

QuLogic · 2022-03-19T08:30:40Z

lib/matplotlib/lines.py

@@ -159,7 +159,24 @@ def _slice_or_none(in_v, slc):
                    "markevery is specified relative to the axes size, but "
                    "the line does not have a Axes as parent")
            # calc cumulative distance along path (in display coords):


This comment doesn't seem to be in the right place to me.

QuLogic · 2022-03-19T08:31:17Z

lib/matplotlib/lines.py

+                aux = np.empty_like(tpath.vertices)
+                for ic, vals in enumerate(tpath.vertices.T):
+                    ok = ~mask[:, ic]
+                    ii = np.where(ok, np.arange(ok.shape[0]), -1)
+                    i0, i1 = np.nonzero(ok)[0][[0, -1]]
+                    ii[:i0] = i0
+                    ilo = np.maximum.accumulate(ii)
+                    ii[mask[:, ic]] = ok.shape[0] - 1
+                    ii[i1:] = i1
+                    ihi = np.minimum.accumulate(ii[::-1])[::-1]
+                    aux[:, ic] = 0.5 * (vals[ilo] + vals[ihi])


This part really needs a comment for explanation; I'm not sure it's the simplest way to achieve what you want since I'm not sure what it's trying to do.

rc · 2022-03-20T12:43:30Z

I have tried to explain the code.

jklymak · 2022-03-28T12:05:43Z

Sorry for being dense, but can you explain this algorithm to me? Naively, I would just do markevery on x, and if a value of y is NaN, then it wouldn't get marked. If I recall correctly there is some visual version of this - so perhaps that is the problem here? Sorry, I need more context without spending a lot of time.

rc · 2022-03-28T12:54:21Z

The patch does nothing to the marking algorithm (which I did not study), it just allows computing the "cumulative distance along path (in display coordinates)" when there are nans in the coordinates. Originally I tried just to remove the nans, but that did not work (data length-related errors). Hmmm. But it should be possible, let me retry.

jklymak · 2022-03-28T13:05:54Z

.. OK, so it is the "in display co-ordinates" that is the problem, caused by passing a float to markevery. Thanks for the reminder. I guess filling in with nonsense y values and then choosing based on that is a good algorithm as any.

rc · 2022-03-28T17:11:08Z

I have just pushed another implementation that just removes the nans. It gives slightly different results, subjectively sometimes not as "good", but the code is trivial and much easier to maintain. Let me know which version you prefer.

jklymak · 2022-03-29T10:01:22Z

lib/matplotlib/lines.py

-            disp_coords = affine.transform(tpath.vertices)
+            fin = np.isfinite(verts).all(axis=1)
+            fverts = verts[fin]
+            disp_coords = affine.transform(fverts)


This seems the easiest, and has the feature of only testing shown markers for visual distance. Not always what folks want, but at least doesn't error, and has the benefit of simplicity.

Are you ok with this @rc or did you want to argue for a different algorithm? @oscargus this changed since your approval. Still OK?

Still OK! I think we have similar views: no error thrown and some output that makes sense (although it is and always will be possible to find cases that gives "sub-optimal" results).

And nicer with a shorter/simpler solution as well.

Yes, this is OK!

@rc can you squash to one commit, and then I will merge... If you have not done a rebase before, make sure to make a backup branch so you can get back to this point. But basically

git fetch upstream git rebase -i upstream/master

and choose squash for all your commits except the first one.

- TST: new test_markevery_linear_scales_nans() test + baseline images

rc · 2022-04-28T09:15:30Z

Re-hi, just checking if the current state is OK.

jklymak · 2022-06-02T14:23:47Z

Sorry to let this languish!

rc · 2022-06-03T09:34:35Z

No problem, thanks for the review!

github-actions bot reviewed Feb 21, 2022

View reviewed changes

oscargus added status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. topic: markers labels Feb 21, 2022

rc force-pushed the fix-float-markevery-with-nans branch from 4557b26 to 08a0eec Compare February 23, 2022 15:37

oscargus approved these changes Mar 6, 2022

View reviewed changes

oscargus added this to the v3.6.0 milestone Mar 6, 2022

QuLogic reviewed Mar 19, 2022

View reviewed changes

jklymak reviewed Mar 29, 2022

View reviewed changes

FIX: allow float markevery with nans in line data in _mark_every_path()

99d9475

- TST: new test_markevery_linear_scales_nans() test + baseline images

rc force-pushed the fix-float-markevery-with-nans branch from 842ad60 to 99d9475 Compare March 30, 2022 13:39

jklymak approved these changes Jun 2, 2022

View reviewed changes

jklymak merged commit 534fbb0 into matplotlib:main Jun 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix markevery plot option with nans in data #22519

fix markevery plot option with nans in data #22519

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix markevery plot option with nans in data #22519

fix markevery plot option with nans in data #22519

Uh oh!

Conversation

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!