-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
FIX: eventplot 'colors' kwarg (#8193) #8204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
e4fe239
c03df1f
c49a4c8
c0c6bc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1314,8 +1314,8 @@ def __init__(self, | |
if positions is None or len(positions) == 0: | ||
segments = [] | ||
elif hasattr(positions, 'ndim') and positions.ndim > 1: | ||
raise ValueError('positions cannot have a dimensionality greater ' | ||
'than 1 (in the ndarray sense)') | ||
raise ValueError('positions cannot be an array with more than ' | ||
'one dimension.') | ||
elif (orientation is None or orientation.lower() == 'none' or | ||
orientation.lower() == 'horizontal'): | ||
positions.sort() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to sort the positions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not totally understand how it works, be it looks like it is needed by how the “offsets” are handled in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the fun I tried to not sorting the positions: the tests do not pass anymore. So I would be inclined to conclude that indeed we need to sort the positions ^^… |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would just write
positions = np.asarray(positions)
(ornp.atleast_1d(positions)
if we want to support scalars)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and then you know for sure it has an ndim attribute)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring does not say that one should expect scalars to be supported, and the other collections classes does not seem to support it either. So I would say =>
np.asarray
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now using
positions = np.asarray(positions)
since fc212ee.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted on a @tacaswell's comment.