Skip to content

Remove axis() manual argument parsing. #24446

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

Merged
merged 1 commit into from
Nov 15, 2022
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 14, 2022

axis is now just a standard function taking 0-1 arguments. This change does make it possible to call axis(None) as synonym for axis(), as well as passing the sole argument under the name arg (unless we decide to switch to positional-only args -- @timhoffm?) but that seems fine.

PR Summary

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@anntzer anntzer force-pushed the axis-argparse branch 2 times, most recently from bda45ba to fda4c05 Compare November 14, 2022 09:18
@timhoffm
Copy link
Member

timhoffm commented Nov 14, 2022

In general, I advocate making the API as narrow as possible so:

  • let's go with positional-only - there's no need to call axis(arg='off'), and I don't think we can find a much better parameter name here. We can always relax that later if a compelling reason comes up.

  • axis(None) is not semantically reasonable. One can prevent that by using a special _DEFAULT sentinel, similar to what we do with UNSET for set():

    class _Unset:
    def __repr__(self):
    return "<UNSET>"
    _UNSET = _Unset()

    (or directly reuse _UNSET, although here the semantics is rather _DEFAULT than _UNSET, so maybe the extra sentinel is semantically justified).
    This is a bit heavy, so I'm really unsure whether accepting None is the pragmatic solution here. OTOH we may have other cases with the same issue and it could be worth making _DEFAULT an explicit concept.

@timhoffm
Copy link
Member

timhoffm commented Nov 14, 2022

BTW: What is axis() doing anyway? AFAICS this results in set_xlim(None, None, emit=True, auto=None) and likewise for ylim, wich does not change anything. - Maybe we should deprecate an empty axis() call and require either a positional or keyword parameter to be set.

@anntzer
Copy link
Contributor Author

anntzer commented Nov 14, 2022

Sure, made it positional only.

axis() with no args is documented to return xmin, xmax, ymin, ymax, which is shorter than calling xlim()/get_xlim() and ylim()/get_ylim() separately; sure, we could deprecate it out of purity but I don't actually think we should -- even if I don't ever use that form, that just seems to be gratuitiously breaking end users (or if we do, we should also get rid of e.g. axis("on")/axis("off") on the grounds that this should be spelled set_axis_on()/set_axis_off()).

Having None be the default and mean the same as no args is pretty standard in Python, and I don't really like the idea of introducing _DEFAULT to strictly keep the same call semantics as previously (for example, distinguishing between None and unset is also the root issue of #24413); Axes.set() is a rather special case as here the signature absolutely cannot be spelled out without introducing such a placeholder.

@jklymak
Copy link
Member

jklymak commented Nov 14, 2022

Maybe we should deprecate an empty axis()

axis returning the limits is a Matlab-ism: https://www.mathworks.com/help/matlab/ref/axis.html

@timhoffm
Copy link
Member

Still a real test failure.

axis is now just a standard function taking 0-1 arguments.  This change
*does* make it possible to call `axis(None)` as synonym for `axis()`, as
well as passing the sole argument under the name `arg` (unless we decide
to switch to positional-only args) but that seems fine.
@anntzer
Copy link
Contributor Author

anntzer commented Nov 14, 2022

oops, fixed.

@ksunden ksunden added this to the v3.7.0 milestone Nov 15, 2022
@ksunden ksunden merged commit 39f40a0 into matplotlib:main Nov 15, 2022
@anntzer anntzer deleted the axis-argparse branch November 15, 2022 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants