From 11d00b5c246af45d52a949e52de9135b7b0e3909 Mon Sep 17 00:00:00 2001 From: Bruno Beltran Date: Fri, 10 Jul 2020 18:27:52 -0700 Subject: [PATCH 1/2] BF: ignore CLOSEPOLY after NaN in PathNanRemover --- lib/matplotlib/tests/test_path.py | 12 ++++++++++++ src/path_converters.h | 13 ++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/lib/matplotlib/tests/test_path.py b/lib/matplotlib/tests/test_path.py index 3ff83da499ff..319f9e78dda5 100644 --- a/lib/matplotlib/tests/test_path.py +++ b/lib/matplotlib/tests/test_path.py @@ -452,3 +452,15 @@ def test_intersect_zero_length_segment(): assert outline_path.intersects_path(this_path) assert this_path.intersects_path(outline_path) + + +def test_cleanup_closepoly(): + # if the first connected component of a Path ends in a CLOSEPOLY, but that + # component contains a NaN, then Path.cleaned should ignore not just the + # control points but also the CLOSEPOLY, since it has nowhere valid to + # point. + p = Path([[np.nan, np.nan], [np.nan, np.nan]], + [Path.MOVETO, Path.CLOSEPOLY]) + cleaned = p.cleaned(remove_nans=True) + assert len(cleaned) == 1 + assert cleaned.codes[0] == Path.STOP diff --git a/src/path_converters.h b/src/path_converters.h index 86d803341d51..b06be01b7a03 100644 --- a/src/path_converters.h +++ b/src/path_converters.h @@ -163,6 +163,7 @@ class PathNanRemover : protected EmbeddedQueue<4> VertexSource *m_source; bool m_remove_nans; bool m_has_curves; + bool valid_segment_exists; public: /* has_curves should be true if the path contains bezier curve @@ -172,7 +173,9 @@ class PathNanRemover : protected EmbeddedQueue<4> PathNanRemover(VertexSource &source, bool remove_nans, bool has_curves) : m_source(&source), m_remove_nans(remove_nans), m_has_curves(has_curves) { - // empty + // ignore all close/end_poly commands until after the first valid + // (nan-free) command is encountered + valid_segment_exists = false; } inline void rewind(unsigned path_id) @@ -202,8 +205,11 @@ class PathNanRemover : protected EmbeddedQueue<4> are found along the way, the queue is emptied, and the next curve segment is handled. */ code = m_source->vertex(x, y); - if (code == agg::path_cmd_stop || - code == (agg::path_cmd_end_poly | agg::path_flags_close)) { + if (code == agg::path_cmd_stop) { + return code; + } + if (code == (agg::path_cmd_end_poly | agg::path_flags_close) + && valid_segment_exists) { return code; } @@ -224,6 +230,7 @@ class PathNanRemover : protected EmbeddedQueue<4> } if (!has_nan) { + valid_segment_exists = true; break; } From e216e59cb219ac68b78301c23ad1ac6faeae3d43 Mon Sep 17 00:00:00 2001 From: Bruno Beltran Date: Mon, 13 Jul 2020 12:46:54 -0700 Subject: [PATCH 2/2] TESTS: ensure removing nan's doesn't break bar --- lib/matplotlib/tests/test_axes.py | 5 +++++ lib/matplotlib/tests/test_path.py | 21 ++++++++++++++++----- src/path_converters.h | 20 ++++++++++++-------- 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/lib/matplotlib/tests/test_axes.py b/lib/matplotlib/tests/test_axes.py index cbc6a5cdd4a5..7e0267c81edd 100644 --- a/lib/matplotlib/tests/test_axes.py +++ b/lib/matplotlib/tests/test_axes.py @@ -1404,6 +1404,11 @@ def test_bar_tick_label_single(): ax.bar("a", "b", align='edge', tick_label='0', data=data) +def test_nan_bar_values(): + fig, ax = plt.subplots() + ax.bar([0, 1], [np.nan, 4]) + + def test_bar_ticklabel_fail(): fig, ax = plt.subplots() ax.bar([], []) diff --git a/lib/matplotlib/tests/test_path.py b/lib/matplotlib/tests/test_path.py index 319f9e78dda5..ebe2e1a46de7 100644 --- a/lib/matplotlib/tests/test_path.py +++ b/lib/matplotlib/tests/test_path.py @@ -459,8 +459,19 @@ def test_cleanup_closepoly(): # component contains a NaN, then Path.cleaned should ignore not just the # control points but also the CLOSEPOLY, since it has nowhere valid to # point. - p = Path([[np.nan, np.nan], [np.nan, np.nan]], - [Path.MOVETO, Path.CLOSEPOLY]) - cleaned = p.cleaned(remove_nans=True) - assert len(cleaned) == 1 - assert cleaned.codes[0] == Path.STOP + paths = [ + Path([[np.nan, np.nan], [np.nan, np.nan]], + [Path.MOVETO, Path.CLOSEPOLY]), + # we trigger a different path in the C++ code if we don't pass any + # codes explicitly, so we must also make sure that this works + Path([[np.nan, np.nan], [np.nan, np.nan]]), + # we should also make sure that this cleanup works if there's some + # multi-vertex curves + Path([[np.nan, np.nan], [np.nan, np.nan], [np.nan, np.nan], + [np.nan, np.nan]], + [Path.MOVETO, Path.CURVE3, Path.CURVE3, Path.CLOSEPOLY]) + ] + for p in paths: + cleaned = p.cleaned(remove_nans=True) + assert len(cleaned) == 1 + assert cleaned.codes[0] == Path.STOP diff --git a/src/path_converters.h b/src/path_converters.h index b06be01b7a03..7c16ae42b9c5 100644 --- a/src/path_converters.h +++ b/src/path_converters.h @@ -205,11 +205,13 @@ class PathNanRemover : protected EmbeddedQueue<4> are found along the way, the queue is emptied, and the next curve segment is handled. */ code = m_source->vertex(x, y); - if (code == agg::path_cmd_stop) { - return code; - } - if (code == (agg::path_cmd_end_poly | agg::path_flags_close) - && valid_segment_exists) { + /* The vertices attached to STOP and CLOSEPOLY left are never + used, so we leave them as-is even if NaN. However, CLOSEPOLY + only makes sense if a valid MOVETO command has already been + emitted. */ + if (code == agg::path_cmd_stop || + (code == (agg::path_cmd_end_poly | agg::path_flags_close) && + valid_segment_exists)) { return code; } @@ -258,7 +260,8 @@ class PathNanRemover : protected EmbeddedQueue<4> code = m_source->vertex(x, y); if (code == agg::path_cmd_stop || - code == (agg::path_cmd_end_poly | agg::path_flags_close)) { + (code == (agg::path_cmd_end_poly | agg::path_flags_close) && + valid_segment_exists)) { return code; } @@ -266,13 +269,14 @@ class PathNanRemover : protected EmbeddedQueue<4> do { code = m_source->vertex(x, y); if (code == agg::path_cmd_stop || - code == (agg::path_cmd_end_poly | agg::path_flags_close)) { + (code == (agg::path_cmd_end_poly | agg::path_flags_close) && + valid_segment_exists)) { return code; } } while (!(std::isfinite(*x) && std::isfinite(*y))); return agg::path_cmd_move_to; } - + valid_segment_exists = true; return code; } }