Repository: matplotlib/matplotlib
Pull request: bugfix for `PathSimplifier`
<!--
Thank you so much for your PR!  To help us review your contribution, please check
out the development guide https://matplotlib.org/devdocs/devel/index.html
-->

## PR summary
<!-- Please provide at least 1-2 sentences describing the pull request in detail
(Why is this change required?  What problem does it solve?) and link to relevant
issues and PRs.

Also please summarize the changes in the title, for example "Raise ValueError on
non-numeric input to set_xlim" and avoid non-descriptive titles such as "Addresses
issue #8576".
-->
closes #17914

`PathSimplifier` should ignore vertices corresponding to `Path.CLOSEPOLY`, and rather replace them with the most recent `Path.MOVETO` vertex.

### Code for reproducing the bug / Actual outcome

```
import numpy as np
import matplotlib.pyplot as plt
from matplotlib.path import Path
from matplotlib.patches import PathPatch

p = Path([(0, 0), (1, 0), (1, 1), (np.nan, np.nan)],
         [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY])

# Remove this to see expected output
p = p.cleaned(simplify=True)

fig, ax = plt.subplots()
patch = PathPatch(p, facecolor="none", edgecolor="blue")
ax.add_patch(patch)
ax.set_xlim(-1, 2)
ax.set_ylim(-1, 2)

plt.show()
```

#### Expected Outcome
The Path should be closed.

<img src="https://github.com/matplotlib/matplotlib/assets/138380708/93487b36-8cd5-4dfa-bef1-78f7ac30e50d" width="400" />

#### Actual Outcome

<img src="https://github.com/matplotlib/matplotlib/assets/138380708/c0d8848b-7fc4-4c4f-82fe-b7bb4510376b" width="400" />


## PR checklist
<!-- Please mark any checkboxes that do not apply to this PR as [N/A].-->

- [x] "closes #0000" is in the body of the PR description to [link the related issue](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
- [x] new and changed code is [tested](https://matplotlib.org/devdocs/devel/testing.html)
- [N/A] *Plotting related* features are demonstrated in an [example](https://matplotlib.org/devdocs/devel/document.html#write-examples-and-tutorials)
- [N/A] *New Features* and *API Changes* are noted with a [directive and release note](https://matplotlib.org/devdocs/devel/api_changes.html#announce-changes-deprecations-and-new-features)
- [N/A] Documentation complies with [general](https://matplotlib.org/devdocs/devel/document.html#write-rest-pages) and [docstring](https://matplotlib.org/devdocs/devel/document.html#write-docstrings) guidelines

<!--We understand that PRs can sometimes be overwhelming, especially as the
reviews start coming in.  Please let us know if the reviews are unclear or
the recommended next step seems overly demanding, if you would like help in
addressing a reviewer's comments, or if you have been waiting too long to hear
back on your PR.-->


Problem statement:
`PathSimplifier` fails to ignore `CLOSEPOLY` vertices
<!--To help us understand and resolve your issue, please fill out the form to the best of your ability.-->
<!--You can feel free to delete the sections that do not apply.-->

### Bug report

**Bug summary**

This bug does not seem to have surfaced anywhere yet, but `Path`'s with `NaN` vertices at `CLOSEPOLY` can come through `PathNanRemover` in a "correct" state but then `PathSimplifier` can sometimes break it:

**Code for reproduction**/**Actual Outcome**

<!--A minimum code snippet required to reproduce the bug.
Please make sure to minimize the number of dependencies required, and provide
any necessary plotted data.
Avoid using threads, as Matplotlib is (explicitly) not thread-safe.-->


```python
>>> p = Path([(0, 0), (1, 0), (1, 1), (np.nan, np.nan)],
         [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY])
>>> # correctly ignores CLOSEPOLY even though it's NaN
>>> p.cleaned(remove_nans=True)
Path(array([[ 0.,  0.],
       [ 1.,  0.],
       [ 1.,  1.],
       [nan, nan],
       [ 0.,  0.]]), array([ 1,  2,  2, 79,  0], dtype=uint8))
>>> # but now for some reason these NaN's are used to populate LINETO's
>>> p.cleaned(remove_nans=True, simplify=True)
Path(array([[ 0.,  0.],
       [ 1.,  0.],
       [ 1.,  1.],
       [nan, nan],
       [nan, nan],
       [ 0.,  0.]]), array([1, 2, 2, 2, 2, 0], dtype=uint8))
```

**Expected outcome**

<!--A description of the expected outcome from the code snippet-->
<!--If this used to work in an earlier version of Matplotlib, please note the version it used to work on-->

The values of the vertices in a `CLOSEPOLY` should always be ignored, in favor of the most recent `MOVETO`'s vertex values:

```python
>>> p.cleaned(remove_nans=True, simplify=True)
Path(array([[ 0.,  0.],
       [ 1.,  0.],
       [ 1.,  1.],
       [0, 0],
       [ 0.,  0.]]), array([1, 2, 2, 2, 0], dtype=uint8))
```

**Matplotlib version**
<!--Please specify your platform and versions of the relevant libraries you are using:-->
  * Operating system:
  * Matplotlib version: 
  * Matplotlib backend (`print(matplotlib.get_backend())`):
  * Python version:
  * Jupyter version (if applicable):
  * Other libraries: 

<!--Please tell us how you installed matplotlib and python e.g., from source, pip, conda-->
<!--If you installed from conda, please specify which channel you used if not the default-->

Hints from issue discussion:
This issue has been marked "inactive" because it has been 365 days since the last comment. If this issue is still present in recent Matplotlib releases, or the feature request is still wanted, please leave a comment and this label will be removed. If there are no updates in another 30 days, this issue will be automatically closed, but you are free to re-open or create a new issue if needed. We value issue reports, and this procedure is meant to help us resurface and prioritize issues that have not been addressed yet, not make them disappear.  Thanks for your help!
I think the bug is in the path simplification dealing with `nan` points (sorry for the non-contiguous numbering, only copied the interesting lines 😉 ) :

```python
In [4]: p = Path([(0, 0), (1, 0), (1, 1), (np.nan, np.nan)],
   ...:          [Path.MOVETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY])
In [9]: p.cleaned(simplify=True)
Out[9]:
Path(array([[ 0.,  0.],
       [ 1.,  0.],
       [ 1.,  1.],
       [nan, nan],
       [nan, nan],
       [ 0.,  0.]]), array([1, 2, 2, 2, 2, 0], dtype=uint8))
In [13]: p.cleaned(simplify=True).cleaned(simplify=True)
Out[13]:
Path(array([[ 0.,  0.],
       [ 1.,  0.],
       [ 1.,  1.],
       [nan, nan],
       [nan, nan],
       [nan, nan],
       [ 0.,  0.]]), array([1, 2, 2, 2, 2, 2, 0], dtype=uint8))

In [14]: p.cleaned(simplify=True).cleaned(simplify=True)
Out[14]:
Path(array([[ 0.,  0.],
       [ 1.,  0.],
       [ 1.,  1.],
       [nan, nan],
       [nan, nan],
       [nan, nan],
       [ 0.,  0.]]), array([1, 2, 2, 2, 2, 2, 0], dtype=uint8))

In [15]: p.cleaned(simplify=True).cleaned(simplify=True).cleaned(simplify=True)
Out[15]:
Path(array([[ 0.,  0.],
       [ 1.,  0.],
       [ 1.,  1.],
       [nan, nan],
       [nan, nan],
       [nan, nan],
       [nan, nan],
       [ 0.,  0.]]), array([1, 2, 2, 2, 2, 2, 2, 0], dtype=uint8))
```

at it seems to insert an extra LINETO nan everytime it is run!

It is not surprising to me that the cleaning steps are not abealian so I am not sure I am too worried that

```python
In [17]: p.cleaned(simplify=True).cleaned(remove_nans=True)
Out[17]:
Path(array([[0., 0.],
       [1., 0.],
       [1., 1.],
       [0., 0.]]), array([1, 2, 2, 0], dtype=uint8))

In [18]: p.cleaned(remove_nans=True).cleaned(simplify=True)
Out[18]:
Path(array([[ 0.,  0.],
       [ 1.,  0.],
       [ 1.,  1.],
       [nan, nan],
       [nan, nan],
       [ 0.,  0.]]), array([1, 2, 2, 2, 2, 0], dtype=uint8))
```

are different and passing both keywords does the second: https://github.com/matplotlib/matplotlib/blob/f800bf61cd863a3d8b4d60268b924a4300ee1815/src/_path.h#L1033-L1045

The implemenation of the simplification is at https://github.com/matplotlib/matplotlib/blob/f800bf61cd863a3d8b4d60268b924a4300ee1815/src/path_converters.h#L694-L912

I suspect that the fix is to add a few lines of special-casing nan because nan is equal to nothing and all math involving nans results in nan so the "is the vector going the same direction enough" logic fails.

I'm marking this as a good first issue (there is no API design and a pretty clear tast case), but has medium difficulty.

To take this issue on you should know both Python and C++ pretty well (including templates, classes, and understanding the `vertex` iterator protocol).  Review on this is likely to be slow.
### Good first issue - notes for new contributors

This issue is suited to new contributors because it does not require understanding of the Matplotlib internals. To get started, please see our [contributing guide](https://matplotlib.org/stable/devel/index).

**We do not assign issues**. Check the *Development* section in the sidebar for linked pull requests (PRs). If there are none, feel free to start working on it. If there is an open PR, please collaborate on the work by reviewing it rather than duplicating it in a competing PR.

If something is unclear, please reach out on any of our [communication channels](https://matplotlib.org/stable/devel/contributing.html#get-connected).

Commit message: simplified test further

Patch to review:
diff --git a/lib/matplotlib/tests/test_path.py b/lib/matplotlib/tests/test_path.py
index 2c4df6ea3b39..c9799f04ad46 100644
--- a/lib/matplotlib/tests/test_path.py
+++ b/lib/matplotlib/tests/test_path.py
@@ -541,3 +541,18 @@ 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
+    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)
diff --git a/src/path_converters.h b/src/path_converters.h
index db732e126c3f..baba45df98e0 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_last_startx(0.0),
+          m_last_starty(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_last_startx = *x;
+                m_last_starty = *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_last_startx;
+                *y = m_last_starty;
+            }
+
             /* 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_last_startx, m_last_starty;
     double m_lastx, m_lasty;
 
     double m_origdx;


Code review comments:
diff_hunk: @@ -541,3 +541,18 @@ 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
+    path_ref = Path([(0, 0), (1, 0), (1, 1), (40, 50)],
path: lib/matplotlib/tests/test_path.py
text: @user1:
A few quick suggestions for enhancing this test:
* Use a different starting location from (0, 0) since that is inserted with the STOP command.
* It would be good to assert against the expected path directly rather than both of these using `simplify=True`
* Should we also see how `simplify=True` interacts with `remove_nans=True`? (see test above this)
* Is there any issue if the first MOVETO is `(nan, nan)` and then we insert it here?
* Could you test this with a compound path with two CLOSEPOLYs? (making sure the correct MOVETO gets inserted for each subpath)

@author:
> Should we also see how `simplify=True` interacts with `remove_nans=True`? (see test above this)

I am not sure what you meant by this. It would be helpful if you could elaborate a bit more.

> Is there any issue if the first MOVETO is `(nan, nan)` and then we insert it here?

Having the first MOVETO as `(nan, nan)` causes it to never initialize the initial vertex, and it ends up closing the polygon to (0, 0) which is wrong.

I think that in order to fix this, it could have a `m_has_init` so that when a CLOSEPOLY is encountered, it will be able to verify that the path has a finite initial point.
```
@@ -757,8 +759,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;
```
```
@@ -773,11 +780,11 @@ class PathSimplifier : protected EmbeddedQueue<9>
             }
             m_after_moveto = false;
 
-            if(agg::is_close(cmd)) {
+            if(agg::is_close(cmd) && m_has_init) {
                 /* If we have to close the polygon, replace
                    the vertex with the starting vertex */
-                *x = m_last_startx;
-                *y = m_last_starty;
+                *x = m_initX;
+                *y = m_initY;
             }
```

Refactoring `m_last_startx` to `m_initX` might be better, as that seems to be the variable name in other path converters (such as `NanRemover`)

@user1:
>> Should we also see how simplify=True interacts with remove_nans=True? (see test above this)

> I am not sure what you meant by this. It would be helpful if you could elaborate a bit more.

I wanted to make sure that we weren't doing too much simplification if you pass `simplify=True` to a test similar to the one above this with only nan's. Making sure you still get back some valid path, which I think your other comment with assuring there is at least a single MOVETO does. So nothing to do here now.