From b2802d13d534f3def4df1a81151fc54d10c8758a Mon Sep 17 00:00:00 2001 From: r3kste <138380708+r3kste@users.noreply.github.com> Date: Sun, 9 Jun 2024 23:13:06 +0530 Subject: [PATCH 1/6] bugfix for path simplifier --- lib/matplotlib/tests/test_path.py | 24 ++++++++++++++++++++++++ src/path_converters.h | 13 +++++++++++++ 2 files changed, 37 insertions(+) diff --git a/lib/matplotlib/tests/test_path.py b/lib/matplotlib/tests/test_path.py index 2c4df6ea3b39..04cdcc301581 100644 --- a/lib/matplotlib/tests/test_path.py +++ b/lib/matplotlib/tests/test_path.py @@ -541,3 +541,27 @@ def test_cleanup_closepoly(): cleaned = p.cleaned(remove_nans=True) assert len(cleaned) == 1 assert cleaned.codes[0] == Path.STOP + + +def test_simplify_closepoly(): + # The values of the vertices in a CLOSEPOLY should always be ignored, + # in favor of the most recent MOVETO's vertex values + paths = ( + Path([(0, 0), (1, 0), (1, 1), (0, 0)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY],), + Path([(0, 0), (1, 0), (1, 1), (np.nan, np.nan)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY],), + Path([(0, 0), (1, 0), (1, 1), (40, 50)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY],), + Path([(0, 0), (0.5, 0), (1, 0), (1, 0.5), + (1, 1), (0.5, 0.5), (0, 0)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, + Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), + ) + + first_segment = list(paths[0].cleaned(simplify=True).iter_segments()) + for p in paths[1:]: + simplified = list(p.cleaned(simplify=True).iter_segments()) + + assert all([np.array_equal(a[0], b[0]) and a[1] == b[1] + for a, b in zip(simplified, first_segment)]) diff --git a/src/path_converters.h b/src/path_converters.h index db732e126c3f..611d37d8fde2 100644 --- a/src/path_converters.h +++ b/src/path_converters.h @@ -644,6 +644,9 @@ class PathSimplifier : protected EmbeddedQueue<9> m_after_moveto(false), m_clipped(false), + m_laststartx(0.0), + m_laststarty(0.0), + // the x, y values from last iteration m_lastx(0.0), m_lasty(0.0), @@ -754,6 +757,8 @@ class PathSimplifier : protected EmbeddedQueue<9> _push(x, y); } m_after_moveto = true; + m_laststartx = *x; + m_laststarty = *y; m_lastx = *x; m_lasty = *y; m_moveto = false; @@ -768,6 +773,13 @@ class PathSimplifier : protected EmbeddedQueue<9> } m_after_moveto = false; + if(agg::is_close(cmd)) { + /* If we have to close the polygon, replace + the vertex with the starting vertex */ + *x = m_laststartx; + *y = m_laststarty; + } + /* NOTE: We used to skip this very short segments, but if you have a lot of them cumulatively, you can miss maxima or minima in the data. */ @@ -919,6 +931,7 @@ class PathSimplifier : protected EmbeddedQueue<9> bool m_moveto; bool m_after_moveto; bool m_clipped; + double m_laststartx, m_laststarty; double m_lastx, m_lasty; double m_origdx; From a4f36fd78c7d41c7cc6be8b678dbd1e8c6cfd73b Mon Sep 17 00:00:00 2001 From: r3kste <138380708+r3kste@users.noreply.github.com> Date: Tue, 25 Jun 2024 12:44:55 +0530 Subject: [PATCH 2/6] simplified test --- lib/matplotlib/tests/test_path.py | 43 +++++++++++++++++-------------- src/path_converters.h | 14 +++++----- 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/lib/matplotlib/tests/test_path.py b/lib/matplotlib/tests/test_path.py index 04cdcc301581..d9e18932d9c3 100644 --- a/lib/matplotlib/tests/test_path.py +++ b/lib/matplotlib/tests/test_path.py @@ -546,22 +546,27 @@ def test_cleanup_closepoly(): def test_simplify_closepoly(): # The values of the vertices in a CLOSEPOLY should always be ignored, # in favor of the most recent MOVETO's vertex values - paths = ( - Path([(0, 0), (1, 0), (1, 1), (0, 0)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY],), - Path([(0, 0), (1, 0), (1, 1), (np.nan, np.nan)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY],), - Path([(0, 0), (1, 0), (1, 1), (40, 50)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY],), - Path([(0, 0), (0.5, 0), (1, 0), (1, 0.5), - (1, 1), (0.5, 0.5), (0, 0)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, - Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), - ) - - first_segment = list(paths[0].cleaned(simplify=True).iter_segments()) - for p in paths[1:]: - simplified = list(p.cleaned(simplify=True).iter_segments()) - - assert all([np.array_equal(a[0], b[0]) and a[1] == b[1] - for a, b in zip(simplified, first_segment)]) + paths = [ + Path([(0, 0), (1, 0), (1, 1), (0, 0)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), + Path([(0, 0), (1, 0), (1, 1), (np.nan, np.nan)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), + Path([(0, 0), (1, 0), (1, 1), (40, 50)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), + Path([(0, 0), (0.5, 0), (1, 0), (1, 0.5), + (1, 1), (0.5, 0.5), (0, 0)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, + Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), + ] + + first_segments = list(paths[0].cleaned(simplify=True).iter_segments()) + for path in paths[1:]: + simplified_segments = list(path.cleaned(simplify=True).iter_segments()) + for seg1, seg2 in zip(simplified_segments, first_segments): + vertex1, code1 = seg1 + vertex2, code2 = seg2 + + # Check whether the vertices are equal + assert_array_equal(vertex1, vertex2) + # Check whether the codes are equal + assert code1 == code2 diff --git a/src/path_converters.h b/src/path_converters.h index 611d37d8fde2..baba45df98e0 100644 --- a/src/path_converters.h +++ b/src/path_converters.h @@ -644,8 +644,8 @@ class PathSimplifier : protected EmbeddedQueue<9> m_after_moveto(false), m_clipped(false), - m_laststartx(0.0), - m_laststarty(0.0), + m_last_startx(0.0), + m_last_starty(0.0), // the x, y values from last iteration m_lastx(0.0), @@ -757,8 +757,8 @@ class PathSimplifier : protected EmbeddedQueue<9> _push(x, y); } m_after_moveto = true; - m_laststartx = *x; - m_laststarty = *y; + m_last_startx = *x; + m_last_starty = *y; m_lastx = *x; m_lasty = *y; m_moveto = false; @@ -776,8 +776,8 @@ class PathSimplifier : protected EmbeddedQueue<9> if(agg::is_close(cmd)) { /* If we have to close the polygon, replace the vertex with the starting vertex */ - *x = m_laststartx; - *y = m_laststarty; + *x = m_last_startx; + *y = m_last_starty; } /* NOTE: We used to skip this very short segments, but if @@ -931,7 +931,7 @@ class PathSimplifier : protected EmbeddedQueue<9> bool m_moveto; bool m_after_moveto; bool m_clipped; - double m_laststartx, m_laststarty; + double m_last_startx, m_last_starty; double m_lastx, m_lasty; double m_origdx; From 24916479e506c206ae000939cba268e963a1543e Mon Sep 17 00:00:00 2001 From: r3kste <138380708+r3kste@users.noreply.github.com> Date: Thu, 27 Jun 2024 00:01:34 +0530 Subject: [PATCH 3/6] simplified test further --- lib/matplotlib/tests/test_path.py | 32 +++++++++---------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/lib/matplotlib/tests/test_path.py b/lib/matplotlib/tests/test_path.py index d9e18932d9c3..c9799f04ad46 100644 --- a/lib/matplotlib/tests/test_path.py +++ b/lib/matplotlib/tests/test_path.py @@ -546,27 +546,13 @@ def test_cleanup_closepoly(): def test_simplify_closepoly(): # The values of the vertices in a CLOSEPOLY should always be ignored, # in favor of the most recent MOVETO's vertex values - paths = [ - Path([(0, 0), (1, 0), (1, 1), (0, 0)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), - Path([(0, 0), (1, 0), (1, 1), (np.nan, np.nan)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), - Path([(0, 0), (1, 0), (1, 1), (40, 50)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), - Path([(0, 0), (0.5, 0), (1, 0), (1, 0.5), - (1, 1), (0.5, 0.5), (0, 0)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, - Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), - ] + path_ref = Path([(0, 0), (1, 0), (1, 1), (40, 50)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]) + path_test = Path([(0, 0), (1, 0), (1, 1), (np.nan, np.nan)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]) + + path_ref_simplified = path_ref.cleaned(simplify=True) + path_test_simplified = path_test.cleaned(simplify=True) - first_segments = list(paths[0].cleaned(simplify=True).iter_segments()) - for path in paths[1:]: - simplified_segments = list(path.cleaned(simplify=True).iter_segments()) - for seg1, seg2 in zip(simplified_segments, first_segments): - vertex1, code1 = seg1 - vertex2, code2 = seg2 - - # Check whether the vertices are equal - assert_array_equal(vertex1, vertex2) - # Check whether the codes are equal - assert code1 == code2 + assert_array_equal(path_ref_simplified.vertices, path_test_simplified.vertices) + assert_array_equal(path_ref_simplified.codes, path_test_simplified.codes) From daf109098aa74d2bc2085856ad4ad4a9215330bf Mon Sep 17 00:00:00 2001 From: r3kste <138380708+r3kste@users.noreply.github.com> Date: Sun, 21 Jul 2024 13:37:21 +0530 Subject: [PATCH 4/6] enhanced tests --- lib/matplotlib/tests/test_path.py | 37 ++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/lib/matplotlib/tests/test_path.py b/lib/matplotlib/tests/test_path.py index c9799f04ad46..3488e378472f 100644 --- a/lib/matplotlib/tests/test_path.py +++ b/lib/matplotlib/tests/test_path.py @@ -546,13 +546,30 @@ def test_cleanup_closepoly(): def test_simplify_closepoly(): # The values of the vertices in a CLOSEPOLY should always be ignored, # in favor of the most recent MOVETO's vertex values - path_ref = Path([(0, 0), (1, 0), (1, 1), (40, 50)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]) - path_test = Path([(0, 0), (1, 0), (1, 1), (np.nan, np.nan)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]) - - path_ref_simplified = path_ref.cleaned(simplify=True) - path_test_simplified = path_test.cleaned(simplify=True) - - assert_array_equal(path_ref_simplified.vertices, path_test_simplified.vertices) - assert_array_equal(path_ref_simplified.codes, path_test_simplified.codes) + paths = [Path([(1, 1), (2, 1), (2, 2), (np.nan, np.nan)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), + Path([(1, 1), (2, 1), (2, 2), (40, 50)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY])] + expected_path = Path([(1, 1), (2, 1), (2, 2), (1, 1), (1, 1), (0, 0)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, + Path.LINETO, Path.STOP]) + + for path in paths: + simplified_path = path.cleaned(simplify=True) + assert_array_equal(expected_path.vertices, simplified_path.vertices) + assert_array_equal(expected_path.codes, simplified_path.codes) + + # test that a compound path also works + path = Path([(1, 1), (2, 1), (2, 2), (np.nan, np.nan), + (-1, 0), (-2, 0), (-2, 1), (np.nan, np.nan)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY, + Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]) + expected_path = Path([(1, 1), (2, 1), (2, 2), (1, 1), + (-1, 0), (-2, 0), (-2, 1), (-1, 0), (-1, 0), (0, 0)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, + Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, + Path.LINETO, Path.STOP]) + + simplified_path = path.cleaned(simplify=True) + assert_array_equal(expected_path.vertices, simplified_path.vertices) + assert_array_equal(expected_path.codes, simplified_path.codes) From 70d0a07303d1db849a26d720296331e9ca4ff71b Mon Sep 17 00:00:00 2001 From: r3kste <138380708+r3kste@users.noreply.github.com> Date: Tue, 23 Jul 2024 12:12:07 +0530 Subject: [PATCH 5/6] moved test and refactored code --- lib/matplotlib/tests/test_path.py | 32 ------------------ lib/matplotlib/tests/test_simplification.py | 32 ++++++++++++++++++ src/path_converters.h | 36 +++++++++++++++------ 3 files changed, 59 insertions(+), 41 deletions(-) diff --git a/lib/matplotlib/tests/test_path.py b/lib/matplotlib/tests/test_path.py index 3488e378472f..2c4df6ea3b39 100644 --- a/lib/matplotlib/tests/test_path.py +++ b/lib/matplotlib/tests/test_path.py @@ -541,35 +541,3 @@ def test_cleanup_closepoly(): cleaned = p.cleaned(remove_nans=True) assert len(cleaned) == 1 assert cleaned.codes[0] == Path.STOP - - -def test_simplify_closepoly(): - # The values of the vertices in a CLOSEPOLY should always be ignored, - # in favor of the most recent MOVETO's vertex values - paths = [Path([(1, 1), (2, 1), (2, 2), (np.nan, np.nan)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), - Path([(1, 1), (2, 1), (2, 2), (40, 50)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY])] - expected_path = Path([(1, 1), (2, 1), (2, 2), (1, 1), (1, 1), (0, 0)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, - Path.LINETO, Path.STOP]) - - for path in paths: - simplified_path = path.cleaned(simplify=True) - assert_array_equal(expected_path.vertices, simplified_path.vertices) - assert_array_equal(expected_path.codes, simplified_path.codes) - - # test that a compound path also works - path = Path([(1, 1), (2, 1), (2, 2), (np.nan, np.nan), - (-1, 0), (-2, 0), (-2, 1), (np.nan, np.nan)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY, - Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]) - expected_path = Path([(1, 1), (2, 1), (2, 2), (1, 1), - (-1, 0), (-2, 0), (-2, 1), (-1, 0), (-1, 0), (0, 0)], - [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, - Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, - Path.LINETO, Path.STOP]) - - simplified_path = path.cleaned(simplify=True) - assert_array_equal(expected_path.vertices, simplified_path.vertices) - assert_array_equal(expected_path.codes, simplified_path.codes) diff --git a/lib/matplotlib/tests/test_simplification.py b/lib/matplotlib/tests/test_simplification.py index 0a5c215eff30..442692d0e2b9 100644 --- a/lib/matplotlib/tests/test_simplification.py +++ b/lib/matplotlib/tests/test_simplification.py @@ -518,3 +518,35 @@ def test_clipping_full(): simplified = list(p.iter_segments(clip=[0, 0, 100, 100])) assert ([(list(x), y) for x, y in simplified] == [([50, 40], 1)]) + + +def test_simplify_closepoly(): + # The values of the vertices in a CLOSEPOLY should always be ignored, + # in favor of the most recent MOVETO's vertex values + paths = [Path([(1, 1), (2, 1), (2, 2), (np.nan, np.nan)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]), + Path([(1, 1), (2, 1), (2, 2), (40, 50)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY])] + expected_path = Path([(1, 1), (2, 1), (2, 2), (1, 1), (1, 1), (0, 0)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, + Path.LINETO, Path.STOP]) + + for path in paths: + simplified_path = path.cleaned(simplify=True) + assert_array_equal(expected_path.vertices, simplified_path.vertices) + assert_array_equal(expected_path.codes, simplified_path.codes) + + # test that a compound path also works + path = Path([(1, 1), (2, 1), (2, 2), (np.nan, np.nan), + (-1, 0), (-2, 0), (-2, 1), (np.nan, np.nan)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY, + Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY]) + expected_path = Path([(1, 1), (2, 1), (2, 2), (1, 1), + (-1, 0), (-2, 0), (-2, 1), (-1, 0), (-1, 0), (0, 0)], + [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, + Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, + Path.LINETO, Path.STOP]) + + simplified_path = path.cleaned(simplify=True) + assert_array_equal(expected_path.vertices, simplified_path.vertices) + assert_array_equal(expected_path.codes, simplified_path.codes) diff --git a/src/path_converters.h b/src/path_converters.h index baba45df98e0..a78643d64ed5 100644 --- a/src/path_converters.h +++ b/src/path_converters.h @@ -644,8 +644,12 @@ class PathSimplifier : protected EmbeddedQueue<9> m_after_moveto(false), m_clipped(false), - m_last_startx(0.0), - m_last_starty(0.0), + // whether the most recent MOVETO vertex is valid + m_has_init(false), + + // the most recent MOVETO vertex + m_initX(0.0), + m_initY(0.0), // the x, y values from last iteration m_lastx(0.0), @@ -757,8 +761,15 @@ class PathSimplifier : protected EmbeddedQueue<9> _push(x, y); } m_after_moveto = true; - m_last_startx = *x; - m_last_starty = *y; + + if (std::isfinite(*x) && std::isfinite(*y)) { + m_has_init = true; + m_initX = *x; + m_initY = *y; + } else { + m_has_init = false; + } + m_lastx = *x; m_lasty = *y; m_moveto = false; @@ -774,10 +785,16 @@ class PathSimplifier : protected EmbeddedQueue<9> m_after_moveto = false; if(agg::is_close(cmd)) { - /* If we have to close the polygon, replace - the vertex with the starting vertex */ - *x = m_last_startx; - *y = m_last_starty; + if (m_has_init) { + /* If we have a valid initial vertex, then + replace the current vertex with the initial vertex */ + *x = m_initX; + *y = m_initY; + } else { + /* If we don't have a valid initial vertex, then + we can't close the path, so we skip the vertex */ + continue; + } } /* NOTE: We used to skip this very short segments, but if @@ -931,7 +948,8 @@ class PathSimplifier : protected EmbeddedQueue<9> bool m_moveto; bool m_after_moveto; bool m_clipped; - double m_last_startx, m_last_starty; + bool m_has_init; + double m_initX, m_initY; double m_lastx, m_lasty; double m_origdx; From 5c6dde09377ede16beca01f565d8f34b3d1d04c7 Mon Sep 17 00:00:00 2001 From: r3kste <138380708+r3kste@users.noreply.github.com> Date: Wed, 24 Jul 2024 01:08:51 +0530 Subject: [PATCH 6/6] added test for edge case --- lib/matplotlib/tests/test_simplification.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/lib/matplotlib/tests/test_simplification.py b/lib/matplotlib/tests/test_simplification.py index 442692d0e2b9..a052c24cb655 100644 --- a/lib/matplotlib/tests/test_simplification.py +++ b/lib/matplotlib/tests/test_simplification.py @@ -550,3 +550,22 @@ def test_simplify_closepoly(): simplified_path = path.cleaned(simplify=True) assert_array_equal(expected_path.vertices, simplified_path.vertices) assert_array_equal(expected_path.codes, simplified_path.codes) + + # test for a path with an invalid MOVETO + # CLOSEPOLY with an invalid MOVETO should be ignored + path = Path([(1, 0), (1, -1), (2, -1), + (np.nan, np.nan), (-1, -1), (-2, 1), (-1, 1), + (2, 2), (0, -1)], + [Path.MOVETO, Path.LINETO, Path.LINETO, + Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, + Path.CLOSEPOLY, Path.LINETO]) + expected_path = Path([(1, 0), (1, -1), (2, -1), + (np.nan, np.nan), (-1, -1), (-2, 1), (-1, 1), + (0, -1), (0, -1), (0, 0)], + [Path.MOVETO, Path.LINETO, Path.LINETO, + Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, + Path.LINETO, Path.LINETO, Path.STOP]) + + simplified_path = path.cleaned(simplify=True) + assert_array_equal(expected_path.vertices, simplified_path.vertices) + assert_array_equal(expected_path.codes, simplified_path.codes)