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

Added warnings for easily confusible subplot/subplots invokations #898

Merged
merged 2 commits into from Jul 18, 2012

Conversation

Projects
None yet
3 participants
Member

WeatherGod commented May 24, 2012

such as::

subplots(1, 2, 1) and subplot(1, 2, False).

@mdboom mdboom and 1 other commented on an outdated diff May 24, 2012

lib/matplotlib/pyplot.py
@@ -860,6 +868,14 @@ def subplots(nrows=1, ncols=1, sharex=False, sharey=False, squeeze=True,
# Four polar axes
plt.subplots(2, 2, subplot_kw=dict(polar=True))
"""
+ # This check was added because it is very easy to type subplots(1, 2, 1)
+ # when subplot(1, 2, 1) was intended. In most cases, no error will
+ # ever occur, but mysterious behavior will result because what was
+ # intended to be the subplot index is instead treated as a bool for
+ # sharex.
+ if not isinstance(sharex, bool) :
+ warnings.warn("sharex argument to subplots() was not boolean."
+ " Did you intend to use subplot()?")
@mdboom

mdboom May 24, 2012

Owner

Why don't we make this a DeprecationWarning -- indicating that it may become an exception in the future and the user is encouraged to change their code. (And same for the warning above).

@pelson

pelson Jun 20, 2012

Member

I'm not sure I agree that this is a deprecation warning (which is hidden by default since python 2.7). It is not the case that this functionality used to work and some structural/procedural change it will mean it will removed in future versions; instead it is a warning to a user to tell them that what they have passed as arguments indicates that they are probably doing the wrong thing.

@mdboom

mdboom Jun 20, 2012

Owner

Yes -- but in the future we may not want to let them do the wrong thing and turn this into an exception -- therefore I think a DeprecationWarning might be appropriate.

Member

pelson commented Jun 20, 2012

#880 is relevant to this issue too.
@WeatherGod would you be prepared to add an explicit check on the type of the incoming args in the case of both subplot(x) and subplot(x, y, z) such that x, y and z must all be ints or np.ints?

Member

pelson commented Jun 20, 2012

This has my +1.

Member

WeatherGod commented Jun 25, 2012

I think I have to rework this code anyway so that it will be put in when pyplot.py gets generated automatically

Member

pelson commented Jul 5, 2012

@WeatherGod: Any update? We may as well put this issue to bed sooner rather than later.

Member

WeatherGod commented Jul 6, 2012

Hmmm, my rebase attempts seemed to be bringing in lots of other stuff. It would probably be better to simply make a new PR.

Member

pelson commented Jul 6, 2012

Agreed. This PR should not be merged in its current state as there are changes in the diffs that are not intended to be part of this PR. Strange that it would merge so strangely.

Member

WeatherGod commented Jul 7, 2012

And I think I fixed the bad rebase. Don't know how that happened, but the commit list now looks reasonable.

Member

WeatherGod commented Jul 18, 2012

Anybody want to merge this, go ahead. Merging my own PR is bad style.

@pelson pelson added a commit that referenced this pull request Jul 18, 2012

@pelson pelson Merge pull request #898 from WeatherGod/subplot_warn
Added warnings for easily confusible subplot/subplots invocations
1ee7e7b

@pelson pelson merged commit 1ee7e7b into matplotlib:master Jul 18, 2012

Member

pelson commented Jul 18, 2012

Anybody want to merge this, go ahead. Merging my own PR is bad style.

Just remember who did the merge when I need such a favour! ;-)

@WeatherGod WeatherGod deleted the WeatherGod:subplot_warn branch Jul 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment