Make sure EventCollection doesn't modify input in-place #14488
Conversation
lib/matplotlib/collections.py
Outdated
| @@ -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: |
There was a problem hiding this comment.
At least this line suggests it was intended that positions could be None, but this PR changes the handling of None.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I don't really know if it can be None or not, could probably use some cleanup :p
There was a problem hiding this comment.
Since passing positions=None would fail later on in some cases on positions.sort(), I've explicitly disallowed None input.
There was a problem hiding this comment.
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... :)
|
|
||
| if positions is None: | ||
| raise ValueError('positions must be an array-like object') | ||
| positions = np.array(positions, copy=True) |
There was a problem hiding this comment.
Technically np.array already copies by default (and there's likely a ton of places in the codebase which rely on that), so just a comment # force a copy is enough.
There was a problem hiding this comment.
I think having copy=True makes it much more clear what is going on, but happy to add a comment too
|
anyone can merge post ci |
|
@meeseeksdev backport to v3.1.x |
…y input in-place
…488-on-v3.1.x Backport PR #14488 on branch v3.1.x (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.