Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Better handling of scalars to plt.subplot(). Fixes #880 #1092

Merged
merged 1 commit into from May 14, 2013

Conversation

Projects
None yet
5 participants
Member

pelson commented Aug 15, 2012

No description provided.

@efiring efiring and 2 others commented on an outdated diff Aug 15, 2012

lib/matplotlib/tests/test_axes.py
@@ -813,6 +814,15 @@ def test_boxplot():
conf_intervals=[None, (-1.0, 3.5)], notch=1)
ax.set_ylim((-30, 30))
+
+@cleanup
+def test_subplot_key_hash():
+ ax = plt.subplot(np.float64(5.5), np.int64(1), np.float64(1.2))
@efiring

efiring Aug 15, 2012

Owner

Would it not make more sense to raise an exception immediately if subplot args are not integer values (though perhaps not necessarily integer dtypes)? I don't see how a value like 5.5 can be anything but a programming error.

@pelson

pelson Aug 16, 2012

Member

I'm inclined to agree. It would break backwards compatibility though, as passing a python float is currently handled by casting to an int. This approach doesn't break that "feature". Preferences?

@mdboom

mdboom Aug 16, 2012

Owner

I think that's a spurious feature at best. I understand it breaks backward compatibility -- but if a float is being passed in, I think it's better to let the user know there was a mistake. We can add a CHANGELOG entry to this effect.

@mdboom mdboom and 2 others commented on an outdated diff Aug 16, 2012

lib/matplotlib/axes.py
@@ -7574,7 +7574,6 @@ def twinx(self):
For those who are 'picking' artists while using twinx, pick
events are only called for the artists in the top-most axes.
"""
-
@mdboom

mdboom Aug 16, 2012

Owner

Can you remove these whitespace only changes in non-relevant files from the commit. (Should be possible with commit amending in git). They make browsing the history more cumbersome.

@pelson

pelson Aug 16, 2012

Member

They are intentional, if not helpful for discussion.

@mdboom

mdboom Aug 17, 2012

Owner

My point was that they are not relevant to the pull request -- they are in files that don't even contain other fixes. Generally, I think white-space only fixes should be in their own commits and PRs.

@efiring

efiring Aug 17, 2012

Owner

Generally, yes, but with the exception that if there are only a few, and they are corrections rather than regressions, and they are in the file in which the substantive changes are being made, then it saves considerable work to leave them in. I don't normally find them until I have already worked with a file, at which point I really don't want to have to separate the deliberate changes from the whitespace changes. As a related point, I think that a moderate amount of unrelated cleanup--the occasional typo or badly-formatted docstring--should be allowed to stay in a commit, if it is not so much as to obscure the meat of the commit. So there is a judgment call involved. Regarding white-space-only changes in files without real fixes, I agree; it is trivial to keep these out of the commit, and take care of them separately.

@mdboom

mdboom Aug 17, 2012

Owner

Yes -- not a hard and fast rule. I guess why these particularly bothered me is that they are in files without any other changes. Doing "git log axes.py" would show that it was edited to improve handling of scalars, when in fact it had nothing to do with it. Just adds a little speed bump when trying to do code forensics down the road.

Member

pelson commented Aug 19, 2012

Realistically, I don't think I can spend the time on this PR to get this into 1.2.x. It's a pretty minor bug which I could live with in a 1.2 release. Agree?

Owner

mdboom commented Aug 19, 2012

As subplot is such a frequently used function, I'd like the benefit of having this in early in case there are any unexpected repercussions. So, yes, we should probably hang off -- but I wouldn't be opposed if it went in a few days late -- we have three weeks lead time until the rc1.

Member

pelson commented Aug 20, 2012

Ok. I will see what can be done.

Member

dmcdougall commented Nov 7, 2012

I'm in favour of raising an exception if the user tries to pass in something that isn't an integer. Since this won't make it into 1.2, it's okay to break backwards compatibility.

On the other hand, it seems that passing nonintegers as indices to arrays does not raise an error:

>>> import numpy as np
>>> a = np.array([[1, 2], [3, 4]])
>>> a[0.1, 1.2]
2

Given that an error is not raised in this case, I can see the argument for supporting noninteger indices. Personally, though, I would never use this 'feature'.

Member

dmcdougall commented Jan 18, 2013

Did we reach a consensus on this?

Owner

efiring commented Jan 18, 2013

I think the consensus was that we should raise an Exception if int(v) != v.

Member

dmcdougall commented Jan 27, 2013

@efiring That's what I thought. Thanks.

I'm not sure the 1.2.1 milestone is appropriate for this PR. Raising an exception if int(v) != v will break code, throwing an error were an error was not thrown before. Granted, I am of the opinion that I do not interpret passing a non-integer value as an index as anything but a programming error, but I feel the user should be warned in advance (e.g., by deprecating first) before making this change.

@ghost ghost assigned pelson Feb 4, 2013

Member

pelson commented Apr 12, 2013

Not sure when it happened, but the problem no longer exists on master (could be from my own PR some time ago which related to projections, but I'm not certain).

This PR is therefore just an addition of a test.

Member

WeatherGod commented Apr 12, 2013

I might have fixed it when I added code to detect if one might have
intended to use subplots() instead of subplot() and vice-versa. I forget
which PR that was, though.

Member

pelson commented Apr 12, 2013

I might have fixed it when I added code to detect if one might have intended to use subplots() instead of subplot() and vice-versa. I forget which PR that was, though.

Cool. Good job! 😄

pelson added a commit that referenced this pull request May 14, 2013

Merge pull request #1092 from pelson/subplot_int
Adds a test for subplot float handling (#880)

@pelson pelson merged commit 47193ee into matplotlib:master May 14, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment