-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Handle NaN in bar labels and error bars #22929
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 all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1083,10 +1083,10 @@ def hlines(self, y, xmin, xmax, colors=None, linestyles='solid', | |||||||||||
lines._internal_update(kwargs) | ||||||||||||
|
||||||||||||
if len(y) > 0: | ||||||||||||
minx = min(xmin.min(), xmax.min()) | ||||||||||||
maxx = max(xmin.max(), xmax.max()) | ||||||||||||
miny = y.min() | ||||||||||||
maxy = y.max() | ||||||||||||
minx = min(np.nanmin(xmin), np.nanmin(xmax)) | ||||||||||||
maxx = max(np.nanmax(xmin), np.nanmax(xmax)) | ||||||||||||
miny = np.nanmin(y) | ||||||||||||
maxy = np.nanmax(y) | ||||||||||||
|
||||||||||||
corners = (minx, miny), (maxx, maxy) | ||||||||||||
|
||||||||||||
|
@@ -1162,10 +1162,10 @@ def vlines(self, x, ymin, ymax, colors=None, linestyles='solid', | |||||||||||
lines._internal_update(kwargs) | ||||||||||||
|
||||||||||||
if len(x) > 0: | ||||||||||||
minx = x.min() | ||||||||||||
maxx = x.max() | ||||||||||||
miny = min(ymin.min(), ymax.min()) | ||||||||||||
maxy = max(ymin.max(), ymax.max()) | ||||||||||||
minx = np.nanmin(x) | ||||||||||||
maxx = np.nanmax(x) | ||||||||||||
miny = min(np.nanmin(ymin), np.nanmin(ymax)) | ||||||||||||
maxy = max(np.nanmax(ymin), np.nanmax(ymax)) | ||||||||||||
|
||||||||||||
corners = (minx, miny), (maxx, maxy) | ||||||||||||
self.update_datalim(corners) | ||||||||||||
|
@@ -2674,7 +2674,7 @@ def sign(x): | |||||||||||
extrema = max(x0, x1) if dat >= 0 else min(x0, x1) | ||||||||||||
length = abs(x0 - x1) | ||||||||||||
|
||||||||||||
if err is None: | ||||||||||||
if err is None or np.size(err) == 0: | ||||||||||||
endpt = extrema | ||||||||||||
elif orientation == "vertical": | ||||||||||||
endpt = err[:, 1].max() if dat >= 0 else err[:, 1].min() | ||||||||||||
|
@@ -3504,7 +3504,9 @@ def apply_mask(arrays, mask): return [array[mask] for array in arrays] | |||||||||||
f"'{dep_axis}err' (shape: {np.shape(err)}) must be a " | ||||||||||||
f"scalar or a 1D or (2, n) array-like whose shape matches " | ||||||||||||
f"'{dep_axis}' (shape: {np.shape(dep)})") from None | ||||||||||||
if np.any(err < -err): # like err<0, but also works for timedelta. | ||||||||||||
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. Could you check for the nan in the conditional? 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.
There may be a better way to write it, but that is the reason for the current approach. 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. Maybe 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. Ah, there will be runtime warnings that will fail the tests.
gives
independent of what is added (unless something like 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. (It took me quite some time to get a solution that worked... Not that it may be a good indication, but at least I failed with quite a few alternatives...) 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.
Yeah, but dtype object isn't guaranteed to do anything under mathematical operations (of particular relevance
Fair, maybe this is a tangent of sorts, but I think we need to fix the underlying problems rather than add more bandaids on top.
I'm 99% sure that if you passed a unit-full pint object in as I'm not blocking on this, but just pointing out that we still have substantial work to do on cleaning up the engineering here. 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 agree that it is bandaid for sure. Thanks! Wasn't aware of pint (although I've seen it mentioned). This is what happens on both
and when using
(empty plot). 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. Fair, so the point is you are definitelynot making it worse here, but the problem still remains. 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 guess that a quick bandaid for matplotlib/lib/matplotlib/axes/_axes.py Lines 3357 to 3361 in f25c2d0
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. No, that is what I'm fundamentally objecting to - just making an object array doesn't make any sense! |
||||||||||||
res = np.zeros_like(err, dtype=bool) # Default in case of nan | ||||||||||||
if np.any(np.less(err, -err, out=res, where=(err == err))): | ||||||||||||
# like err<0, but also works for timedelta and nan. | ||||||||||||
raise ValueError( | ||||||||||||
f"'{dep_axis}err' must not contain negative values") | ||||||||||||
# This is like | ||||||||||||
|
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.
Since these are already combining/using masks above, would it make sense to add a
masked_invalid()
call earlier?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.
I do not really know how masks actually works (which is a bit ironically, considering my comment on your PR about masks I did just before seeing this...), although I realize that it may be related. Just noted that this didn't work correctly when trying out the other fixes.
I may give it a do though, either here or as a follow-up.
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.
👍 There is a
cbook.safe_masked_invalid
if that would help at all. Masking the invalid locations just ignores those in the computations (well the details are more involved, but that is the easy way of thinking about it).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.
I've played around with it and cannot get it to work. The reason seems to be that the array has
dtype=object
(although it is float when created) and therefore I get runtime warnings onmin
andmax
.Using
masked_invalid
gives a masked array with a fill_value of?
, whilecbook.safe_masked_invalid
just returns an identical array.If the array was
float
it would probably work though, but I have no idea where it is becomingdtype=object
. Maybe related todatetime
support?(Not really clear to me why
nanmin
andnanmax
is worse than adding masked conversions.)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.
I don't think there is necessarily anything wrong with it if we are expecting nan's for some reason. To me, this just seemed like it was potentially hiding something else, which as you've noted here this only happens for
dtype=object
. If you are passing in a float array, I really don't think we should be making it an object array 😟 Perhaps this is because these error bars are generally used for categoricals so something is converting to objects fast because of that?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.
We do this because we don't want to drop units.