The Wayback Machine - https://web.archive.org/web/20211030072835/https://github.com/matplotlib/matplotlib/pull/14993
Skip to content
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

Remove custom_projection example. #14993

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@anntzer
Copy link
Contributor

@anntzer anntzer commented Aug 6, 2019

It's basically an annotated version of (parts of) geo.py, that is slowly
getting out of sync with the main implementation. Kill it, and instead
move the relevant comments to geo.py (this also caught a few typos in
the comments previously in geo.py).

PR Summary

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
lib/matplotlib/projections/geo.py Show resolved Hide resolved
lib/matplotlib/projections/geo.py Outdated Show resolved Hide resolved
@ImportanceOfBeingErnest
Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest commented Aug 6, 2019

The example still works fine. If it needs to be updated, that can be done. Since this is the only example of custom projections with a non-affine component and non-rectangular boundaries, it is pretty useful. The fact that it is largely similar to some part of matplotlib's internal code is no argument to remove it, because it's hard enough to understand it in the stand-alone case - if people are then in addition required to find out which parts of the source code they do need and which others they don't, it will be even harder.

@anntzer
Copy link
Contributor Author

@anntzer anntzer commented Aug 6, 2019

I would rather make the library version easier to read (further improvements are welcome) and point users to it (the rendered docs have a [source] link, after all) rather than having two subtly different implementations of the same thing.

It's basically an annotated version of (parts of) geo.py, that is slowly
getting out of sync with the main implementation.  Kill it, and instead
move the relevant comments to geo.py (this also caught a few typos in
the comments previously in geo.py).
@timhoffm
Copy link
Member

@timhoffm timhoffm commented Aug 7, 2019

+/-0 on removing. The example is still hard to learn from. A simpler example would be better. Could also benefit from some sectioning and explanatory texts (sphinx-gallery) not only code comments. But we don't have that either.

@story645
Copy link
Member

@story645 story645 commented Aug 7, 2019

I agree with @timhoffm. The current example is too complicated for most people to learn from, so may as well not have the maintainance burden of keeping it in docs. Is there a use case of a custom projection that would make a good toy example?

@ImportanceOfBeingErnest
Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest commented Aug 7, 2019

One shouldn't underestimate the usecase where you just copy, paste and run an example without understanding it and then just replace bits and pieces until you get your desired result ("learning by doing", "training-on-the-job").
I therefore stressed that it is important that this example uses a non-affine transform and non-rectangular boundaries.
(An example using non-custom, affine transforms and a regular boundary would be SkewT-logP diagram: using transforms and custom projections)

Clearly, if someone has a better idea for this, other than the geographic use case we currently have, that would work as well. (But maybe it's also easier to maintain an example which is close to the source code than one that is further away?)

@anntzer
Copy link
Contributor Author

@anntzer anntzer commented Aug 7, 2019

One shouldn't underestimate the usecase where you just copy, paste and run an example without understanding it and then just replace bits and pieces until you get your desired result ("learning by doing", "training-on-the-job").

We could certainly even split the various geo projections implementations each in their own file to make them independently copypastable. And even then, right now you can just open geo.py and copy its first 305 lines and get a self-standing implemenation of Aitoff projection (that's just because it's the first one there) that you can play with.

(But maybe it's also easier to maintain an example which is close to the source code than one that is further away?)

The point is that they have not been maintained together: they have drifted apart throughout the years.

@tacaswell tacaswell added this to the v3.3.0 milestone Aug 12, 2019
@QuLogic QuLogic removed this from the v3.3.0 milestone May 2, 2020
@QuLogic QuLogic added this to the v3.4.0 milestone May 2, 2020
@jklymak jklymak marked this pull request as draft Aug 24, 2020
@QuLogic QuLogic removed this from the v3.4.0 milestone Jan 21, 2021
@QuLogic QuLogic added this to the v3.5.0 milestone Jan 21, 2021
@QuLogic QuLogic removed this from the v3.5.0 milestone Aug 23, 2021
@QuLogic QuLogic added this to the v3.6.0 milestone Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment