Conversation
| angle = 0. | ||
| else: | ||
| angle = math.degrees(math.atan2(dy, dx)) | ||
| angle = math.degrees(math.atan2(dy, dx)) |
There was a problem hiding this comment.
math.atan2(0, 0) (or likewise with floats) returns 0 or pi depending on the signedness of the zeros in Py2 and Py3 and that's also mandated by the C standard. norm_text_angle later takes it modulo 180 so we're fine.
| # with az = -54 and elev = -45. | ||
| return np.min(tzs) | ||
| else : | ||
| else: |
There was a problem hiding this comment.
else:
return np.min(tzs) if tzs.size else np.nan
There was a problem hiding this comment.
I see where you're coming from but mixing in a ternary in a if... elif chain looks meh to me.
There was a problem hiding this comment.
Actually, I don't think that the three blocks are logically equal. To me, the current form
if self._sort_zpos is not None:
[use _sort_zpos]
elif tzs.size > 0:
[use min(tzs)]
else:
[use fallback]
is logically more like
if self._sort_zpos is not None:
[use _sort_zpos]
else:
if tzs.size > 0:
[use min(tzs)]
else:
[use fallback]
Usually tzs are expected to be there and nan is only a fallback if they are not. Thus it would be preferable to
if self._sort_zpos is not None:
[use _sort_zpos]
else:
[use min(tzs) with fallback]
However, I agree that depends on the point of view you take. And I won't argue further if you still prefer to leave it as is.
There was a problem hiding this comment.
I know (hence the "I see where you're coming from" above), but it still looks not so nice to me. (Really, best would be min(tzs, default=np.nan) but np.min doesn't have that and builtins.min is likely slower.)
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
| else : | ||
| zsortval = 0 # FIXME: Fairly arbitrary. Is there a better value? | ||
| zsortval = ( | ||
| np.min(zvals) if zvals.size else 0) # FIXME: arbitrary default |
There was a problem hiding this comment.
Slightly better:
default_z = 0 # FIXME: arbitrary default
zsortval = np.min(zvals) if zvals.size else default_z
There was a problem hiding this comment.
went for some middle ground
| """ | ||
| ax.quiver(X, Y, Z, U, V, W, /, length=1, arrow_length_ratio=.3, pivot='tail', normalize=False, **kwargs) | ||
| ax.quiver(X, Y, Z, U, V, W, /, length=1, arrow_length_ratio=.3, \ | ||
| pivot='tail', normalize=False, **kwargs) |
There was a problem hiding this comment.
Can this be indented to align with the bracket in the line above?
There was a problem hiding this comment.
not really, that'll look awkward in the actual docstring (the problem is the same as always re: line continuations in rst).
There was a problem hiding this comment.
I think treating the first line of the docstring as a replacement __signature__ is a sphinx thing, not an rst thing - it's possible it gets handled before the RST parser kicks in.
|
Thanks @anntzer ! |
PR Summary
PR Checklist