Skip to content

Document Transform.__add__ and .__sub__. #17218

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 2 commits into from
Apr 22, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Apr 22, 2020

PR Summary

Closes #17217.

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

@anntzer anntzer added this to the v3.3.0 milestone Apr 22, 2020
@anntzer
Copy link
Contributor Author

anntzer commented Apr 22, 2020

@QuLogic the link to circleci artefacts is broken?

@ImportanceOfBeingErnest
Copy link
Member

Maybe important to note that (A - B) + B is not the same as (A + B.inverted()) + B because .inverted() returns a frozen copy of the transform.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 22, 2020

Ah yes, very good point. I pushed a wording which may be a bit awkward though, feel free to propose a better phrasing.

@QuLogic
Copy link
Member

QuLogic commented Apr 22, 2020

@anntzer Yes, it's because you have CircleCI enabled on your fork scientific-python/circleci-artifacts-redirector-action#7.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 22, 2020

Ah ok, disabled it then, and fixed the typo.

@jklymak
Copy link
Member

jklymak commented Apr 22, 2020

This is very helpful. A couple of suggestions to more fully close #17217.

  1. Can some of this move up to the top of transofrms.py?
  2. Can __add__ get a bit of work as well?

@anntzer
Copy link
Contributor Author

anntzer commented Apr 22, 2020

__add__ is really just apply A then B :-) so I don't have much idea of what else to write, but if you have any better wording to suggest, please go for it.

@jklymak
Copy link
Member

jklymak commented Apr 22, 2020

Yes, and that was not crystal clear from the doctoring.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 22, 2020

"doctoring" is an interesting autocorrect for "docstring" :-)

@jklymak
Copy link
Member

jklymak commented Apr 22, 2020

Oops. Anyhoo, sorry for the flake8 can you fix if you approve because the web editor is apparently unable to stop itself from inserting lots of spaces

... the order of operations is really not clear from the existing doc string.
@anntzer
Copy link
Contributor Author

anntzer commented Apr 22, 2020

done with a small edit.

@jklymak jklymak merged commit 7125492 into matplotlib:master Apr 22, 2020
@jklymak
Copy link
Member

jklymak commented Apr 22, 2020

Thanks definitely an improvement. I still think a quick blurb at the top of the module would help...

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

Successfully merging this pull request may close these issues.

Transform operators are not publicly documented....
4 participants