8000 ENH: Add boolean support for axis() by abhinuvpitale · Pull Request #12359 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

ENH: Add boolean support for axis() #12359

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 5, 2019
Merged

ENH: Add boolean support for axis() #12359

merged 1 commit into from
Mar 5, 2019

Conversation

abhinuvpitale
Copy link
Contributor
@abhinuvpitale abhinuvpitale commented Oct 1, 2018 •
edited
Loading

PR Summary

Closes #12311

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@ImportanceOfBeingErnest ImportanceOfBeingErnest changed the title add boolean support for axis() for issue #12311 ENH: Add boolean support for axis() Oct 1, 2018
@ImportanceOfBeingErnest
Copy link
Member

Thanks. Now if you test this you would see that it produces an error. the reason is that a boolean cannot be indexed. So you may put everything that comes behind the newly added block into an else clause, or you may return inside the if clause. Or you put this into the first if from that function.

In addition you need to make sure the correct limits are returned. You could directly copy lines

matplotlib/lib/matplotlib/axes/_base.py

Lines 1686 to 1688 in e7438c3

xmin, xmax = self.get_xlim()
ymin, ymax = self.get_ylim()
return xmin, xmax, ymin, ymax
in case you use either of the first two options.

To make sure this does what it should, a test would need to be added. Probably inside matplotlib/lib/matplotlib/tests/test_axes.py.

Finally, the new option would need to be documented. In the docstring of the axis function it needs to mention that it can take a boolean to toggle the axes on or off.

@abhinuvpitale
Copy link
Contributor Author

Oh yes, I think I get the changes now, will complete then over this week!

@abhinuvpitale
Copy link
Contributor Author 
if len(v) == 1 and isinstance(v, bool):

Should I use this check?

@abhinuvpitale
Copy link
Contributor Author 
if len(v) == 1 && isinstance(v, bool):
            if v:
                self.set_axis_on()
            else:
                self.set_axis_off()
    xmin, xmax = self.get_xlim() 
    ymin, ymax = self.get_ylim() 
    return xmin, xmax, ymin, ymax

Does this look okay?

@ImportanceOfBeingErnest
Copy link
Member

A bool has no length. len(True) results in an error. But there is no need for a length comparisson here anyways.

@ImportanceOfBeingErnest
Copy link
Member
ImportanceOfBeingErnest commented Oct 5, 2018 •
edited
Loading

Right, I think this should work now. Because I think is always hard to judge on, the usual way we handle this is to create a test for new features.

Here it seems a decent way to test is that a figure with an axes and ax.axis("off") and ax.axis(False) produce the same output, similar for "on"/True afterwards. This could be done with a check_figures_equal decorator. For example, see

matplotlib/lib/matplotlib/tests/test_axes.py

Lines 5742 to 5743 in 46ba49a

@check_figures_equal(extensions=["png"])
def test_markerfacecolor_none_alpha(fig_test, fig_ref):

Or, you may just check if the figure is created without error and asset the visibility.

Finally, the documentation will of course need to be upated to reflect the new arguments.

@abhinuvpitale
Copy link
Contributor Author

I will need a little bit of help with the test, how are the two arguments passed to the function in the examples you showed above

This is a basic non-error out test

# Test to check if it doesnt error out
def test_axis_bool_arguments():
  fig,ax = plt.subplots()
  ax.axis(True)
  ax.axis(False)

Can you tell how to use the decorator?

@ImportanceOfBeingErnest
Copy link
Member
ImportanceOfBeingErnest commented Oct 7, 2018 •
edited
Loading

I would have thought something like this:

@check_figures_equal(extensions=["png"]) 
def test_axis_bool_arguments(fig_test, fig_ref):
    # Test if False and "off" give the same
    fig_test.add_subplot(211).axis(False)
    fig_ref.add_subplot(211).axis("off")
    # Test if True after False gives the same as the default
    ax = fig_test.add_subplot(212)
    ax.axis(False)
    ax.axis(True)
    fig_ref.add_subplot(212)

@abhinuvpitale
Copy link
Contributor Author
abhinuvpitale commented Oct 7, 2018 •
edited
Loading

did you miss a fig_test.add_subplot(212).axis(True) at the end?

@abhinuvpitale
Copy link
Contributor Author

nvm, the default is true!
My bad

@ImportanceOfBeingErnest
Copy link
Member

Sorry, you're right, the last line should not set anything on. I edited that post.

@abhinuvpitale
Copy link
Contributor Author

@ImportanceOfBeingErnest But shouldn't we be testing the True argument also?
PLease check the commit I made, I think thats how I intend it to be

@ImportanceOfBeingErnest
Copy link
Member

The second last line tests if True works correctly. You have an indendation error in the current version.

@abhinuvpitale
Copy link
Contributor Author

could you help me with the ci tests?
I am not sure what's failing!

timhoffm
timhoffm reviewed Oct 7, 2018
View reviewed changes
@abhinuvpitale
Copy link
Contributor Author

Thanks for the clarification @timhoffm

@abhinuvpitale
Copy link
Contributor Author

Slightly unsure what the errors are!

anntzer
anntzer reviewed Oct 8, 2018
View reviewed changes
lib/matplotlib/axes/_base.py Outdated
Optional positional-only argument

If a bool,

======== ==========================================================
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A table here seems overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I could remove the table!
Though that was just to preserve the formatting similar to that already on the page!

8000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say,

If a bool, turns axis lines and labels on or off.

Later inside the string table you can also refer to it

             Value    Description
             ======== ==========================================================
             'on'     Turn on axis lines and labels. Same as ``True``.
             'off'    Turn off axis lines and labels. Same as ``False``.

anntzer
anntzer reviewed Oct 8, 2018
View reviewed changes
@ImportanceOfBeingErnest ImportanceOfBeingErnest added this to the v3.1 milestone Oct 9, 2018
anntzer
anntzer reviewed Oct 9, 2018
View reviewed changes
lib/matplotlib/axes/_base.py Outdated
@@ -1595,7 +1595,7 @@ def axis(self, *v, **kwargs):

Parameters
----------
v : List[float] or one of the strings listed below.
v : List[float] or bool or one of the strings listed below.
Optional positional-only argument

If a list, set the axis data limits. If a string:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if a string or a bool

and below in the table

'on' or True
'off' or False

@abhinuvpitale
Copy link
Contributor Author

I think this can be merged and closed, right?

@ImportanceOfBeingErnest
Copy link
Member

I think the error is unrelated, but someone may recheck.
Would it be possible to squash the 14 commits into a single one?

@timhoffm
Copy link
Member

@ImportanceOfBeingErnest we have a grafik button 😄

@abhinuvpitale
Copy link
Contributor Author

@timhoffm haha thanks for the button, I am guessing I don't need to squash them? 🧀

@ImportanceOfBeingErnest
Copy link
Member

I think you need to fetch upstream first, then run boilerplate.py. Possibly you need to rebase.

@ImportanceOfBeingErnest
Copy link
Member

Something went wrong here. You now have 44 commits in your PR.

@abhinuvpitale
Copy link
Contributor Author

Yeah, I am worried now! Have to rebase back

@abhinuvpitale
Copy link
Contributor Author

Hi, Sorry I was not able to get back to this! I think I did an incorrect rebase and these commits showed up. I think I will go pull the latest master and make the changes again

@tacaswell
Copy link
Member

@abhinuvpitale Let us know if you need help with the rebase (it may be simpler to cherry-pick the commits you want to keep).

When you get a local branch you like, you can 'force-push' to this branch on github and this PR will update to reflect those changes (git push --force-with-lease).

@abhinuvpitale
Copy link
Contributor Author

@tacaswell Yeah, could you help me with this rebase? I know which commits to cherry pick, though I am not sure if

  1. I should create a new clone on current master or
  2. Continue with the current branch?

@tacaswell
Copy link
Member

@abhinuvpitale May I force-push to your branch?

@abhinuvpitale
Copy link
Contributor Author

Yeah sure @tacaswell ! Also please let me know what you did to get it to a clean state.!

@abhinuvpitale
Copy link
Contributor Author

@tacaswell any update?

@anntzer
Copy link
Contributor
anntzer commented Dec 31, 2018

Perhaps this should also include np.bool_?

@jklymak jklymak modified the milestones: v3.1.0, v3.2.0 Feb 22, 2019
@QuLogic
Copy link
Member
QuLogic commented Mar 5, 2019

I've pushed a squashed version of this, and fixed the boilerplate.

@abhinuvpitale please fetch before working on this (unless it's already merged.)

@anntzer anntzer merged commit 2834743 into matplotlib:master Mar 5, 2019
@tacaswell
Copy link
Member

@abhinuvpitale Sorry, I dropped the ball on this.

@Atcold
Copy link
Atcold commented Mar 5, 2019

Yay! Thanks everyone for fixing #12311!

@timhoffm
Copy link
Member
timhoffm commented Mar 5, 2019

Should we backport to 3.1?

@QuLogic
Copy link
Member
QuLogic commented Mar 5, 2019

I think that was the original plan, so why not.

@meeseeksdev backport to v3.1.x

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 5, 2019
@anntzer
Backport PR matplotlib#12359: ENH: Add boolean support for axis()
6fdc027
@meeseeksmachine meeseeksmachine mentioned this pull request Mar 5, 2019
Merged
@QuLogic QuLogic modified the milestones: v3.2.0, v3.1.0 Mar 5, 2019
@abhinuvpitale abhinuvpitale deleted the abhinuvpitale-patch-issue12311 branch March 6, 2019 05:38
timhoffm added a commit that referenced this pull request Mar 6, 2019
@timhoffm
Merge pull request #13597 from meeseeksmachine/auto-backport-of-pr-12…
e42b813 
…359-on-v3.1.x

Backport PR #12359 on branch v3.1.x (ENH: Add boolean support for axis())
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak left review comments

@timhoffm timhoffm timhoffm approved these changes

@ImportanceOfBeingErnest ImportanceOfBeingErnest ImportanceOfBeingErnest left review comments

@anntzer anntzer anntzer approved these changes

Assignees

@tacaswell tacaswell

Labels
None yet
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.

'off' vs. False bug
9 participants
@abhinuvpitale @ImportanceOfBeingErnest @timhoffm @jklymak @QuLogic @tacaswell @anntzer @Atcold @dstansby
0