Repository: matplotlib/matplotlib
Pull request: Make sure EventCollection doesn't modify input in-place
Fixes #14487. Not entirely sure if casting any input to an array is correct, but will see if the tests all pass.

Problem statement:
eventplot sorts np.array positions, but not list positions 
<!--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**

When using eventplot, the data is sorted in-place if it's passed as a numpy array. If the data is passed as a list, this doesn't happen. 

**Code for reproduction**

```python
from matplotlib import pyplot as plt
import numpy as np

# This sorts the data
sample_data = np.array([3, 2, 1, 10])
plt.eventplot(sample_data)
print(sample_data)

# This does not sort the data
sample_data = [3, 2, 1, 10]
plt.eventplot(sample_data)
print(sample_data)
```

**Actual outcome**

```
[ 1  2  3 10]
[3, 2, 1, 10]

```

**Expected outcome**
For consistency, it would be good if the outcome was always sorted, or always untouched. My preference would be that the data remains unsorted.

**Matplotlib version**
<!--Please specify your platform and versions of the relevant libraries you are using:-->
  * Operating system: Ubuntu 16.04.4
  * Matplotlib version: 3.0.2
  * Matplotlib backend (`print(matplotlib.get_backend())`): module://ipykernel.pylab.backend_inline
  * Python version: 3.7.2
  * Jupyter version (if applicable): jupyterlab version 0.35.4
  * Other libraries: numpy version 1.16.2

All libraries were installed using conda.

Hints from issue discussion:

Commit message: Make sure EventCollection doesn't modify in put inplace

Patch to review:
diff --git a/lib/matplotlib/collections.py b/lib/matplotlib/collections.py
index 54d867c394f5..574acec01c58 100644
--- a/lib/matplotlib/collections.py
+++ b/lib/matplotlib/collections.py
@@ -1462,7 +1462,7 @@ def __init__(self,
 
         .. plot:: gallery/lines_bars_and_markers/eventcollection_demo.py
         """
-
+        positions = np.array(positions, copy=True)
         segment = (lineoffset + linelength / 2.,
                    lineoffset - linelength / 2.)
         if positions is None or len(positions) == 0:
diff --git a/lib/matplotlib/tests/test_collections.py b/lib/matplotlib/tests/test_collections.py
index 29009944a16e..c0da585a9a2a 100644
--- a/lib/matplotlib/tests/test_collections.py
+++ b/lib/matplotlib/tests/test_collections.py
@@ -711,3 +711,10 @@ def test_pathcollection_legend_elements():
         ax.add_artist(l)
 
     fig.canvas.draw()
+
+
+def test_EventCollection_nosort():
+    # Check that EventCollection doesn't modify input in place
+    arr = np.array([3, 2, 1, 10])
+    coll = EventCollection(arr)
+    np.testing.assert_array_equal(arr, np.array([3, 2, 1, 10]))


Code review comments:
diff_hunk: @@ -1462,7 +1462,7 @@ def __init__(self,
 
         .. plot:: gallery/lines_bars_and_markers/eventcollection_demo.py
         """
-
+        positions = np.array(positions, copy=True)
         segment = (lineoffset + linelength / 2.,
                    lineoffset - linelength / 2.)
         if positions is None or len(positions) == 0:
path: lib/matplotlib/collections.py
text: @user1:
At least this line suggests it was intended that `positions` could be None, but this PR changes the handling of None.

@author:
Huh, you're right, even if there is a comment above (https://github.com/matplotlib/matplotlib/pull/14488/files#diff-506c6bd01694bddbd8999f2c6e920705R1408) that implies `positions` can't be `None`. I'll fix it to keep the current behaviour with None though (and add a smoke test for `positions=None`)

@user1:
I don't really know if it can be None or not, could probably use some cleanup :p

@author:
Since passing `positions=None` would fail later on in some cases on `positions.sort()`, I've explicitly disallowed `None` input.

@user1:
It would previously not fail because the sort() is in another else clause; indeed `gca().add_collection(EventCollection(None))` works.
On the other hand there's basically not benefit to supporting None as input (in particular `eventplot(None)` already doesn't work) so I'm fine with dropping that, won't even insist on an api changes note... :)