The Wayback Machine - https://web.archive.org/web/20200716202947/https://github.com/matplotlib/matplotlib/pull/16457
Skip to content

/ matplotlib

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

Build lognorm/symlognorm from corresponding scales. #16457

Open
wants to merge 1 commit into
base: master
from
Open

Build lognorm/symlognorm from corresponding scales. #16457

wants to merge 1 commit into from

Conversation

@anntzer
Copy link
Contributor

anntzer commented Feb 10, 2020

Infer the (Sym)LogNorm math from the one in the corresponding scales. See #16375 for the need for it. (In my view, we probably could have done without norms at all, a norm is basically "just" a scale on the colorbar... anyways)

This fails test_colors.py::test_SymLogNorm because the old symlog norm
and scale were not consistent (as discussed extensively elsewhere -- this needs to be resolved in a way or another).

test_contour::test_contourf_log_extension fails due to a tick moving by 1px, but the new result actually looks nicer IMO.

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
@anntzer
anntzer reviewed Feb 10, 2020
View changes
lib/matplotlib/colors.py Outdated
return functools.partial(_make_norm_from_scale, scale_cls, init=init)

if init is None:
init = lambda vmin=None, vmax=None, clip=False: None

This comment has been minimized.

Sign in to view
Copy link
@anntzer

anntzer Feb 10, 2020

Author Contributor

the signature overriding mess is because the signatures of SymLogScale and SymLogNorm are not consistent with one another either...
@jklymak
Copy link
Contributor

jklymak commented Feb 10, 2020

See #16391 for related discussion and issues.

Overall this seems like a great help to me. Hopefully if we have more norms in the future, authors will create a scale and just use this factory. Probably could do this for TwoSlopeNorm.
@jklymak jklymak added this to the v3.3.0 milestone Feb 10, 2020
@jklymak jklymak self-assigned this Feb 24, 2020
@anntzer
Copy link
Contributor Author

anntzer commented Feb 25, 2020

The need for rebase comes from the deprecation that went into SymLogNorm re: base change, which the decorator obviously can't know about. I think trying to shoehorn the deprecation into the decorator is going to make things waaaay more complicated than needed, and we may just accept to have a fully separate SymLogNorm until the deprecation is resolved, keeping the decorator only for LogNorm for now.
@jklymak
Copy link
Contributor

jklymak commented Feb 25, 2020

I guess. Or drop the deprecation warning. Based on the conversations, folks were using this qualitatively and might not complain about the base change.
@anntzer
Copy link
Contributor Author

anntzer commented Feb 25, 2020

Let's pick one strategy or the other before I actually do the work of rebasing? :)
@jklymak
Copy link
Contributor

jklymak commented Feb 26, 2020

#16404 is 3.2, so doing away w/ the deprecation should be fine for 3.3?
@anntzer
Copy link
Contributor Author

anntzer commented Feb 26, 2020

Are we fine with breaking after just one release?
@jklymak
Copy link
Contributor

jklymak commented Feb 26, 2020

I think it was broken before the fix so warning we are fixing a bug for one release seems generous. Ie the function didn’t do what it’s docstring said.
@anntzer anntzer removed the Needs rebase label Feb 26, 2020
@anntzer anntzer force-pushed the anntzer:scale-norm branch from bb3d7fc to af8eb61 Feb 26, 2020
@anntzer
Copy link
Contributor Author

anntzer commented Feb 26, 2020

OK, rebased. Still needs API changes note re: breakage.
Now there's just one baseline image that changes -- a single colorbar tick that moves by one pixel. Dunno how this plays with #16286.
@jklymak
jklymak approved these changes Feb 27, 2020
View changes
lib/matplotlib/colors.py
ba.apply_defaults()
super().__init__(
**{k: ba.arguments.pop(k) for k in ["vmin", "vmax", "clip"]})
self._scale = scale_cls(axis=None, **ba.arguments)

This comment has been minimized.

Sign in to view
Copy link
@jklymak

jklymak Feb 27, 2020

Contributor

This looks great to me. For colorbar we will need access to self._scale, which I assume this is where we get this.

This comment has been minimized.

Sign in to view
Copy link
@anntzer

anntzer Feb 27, 2020

Author Contributor

Yes.
@anntzer
Copy link
Contributor Author

anntzer commented Feb 27, 2020

Can you write the api break note? ;)
@jklymak
Copy link
Contributor

jklymak commented Feb 27, 2020 •
edited 

`SymLogNorm` was documented to have a base=10 for the logarithmic part 
of the scale, but the code had `base=np.e`.  The default is now `base=10` in 
accordance with the documentation; to get the old behaviour, pass the optional 
`base=np.e` to the norm.
@jklymak
Copy link
Contributor

jklymak commented Feb 27, 2020

I think we still have a todo wrt SymLogNorm's documentation, but thats not really this PR's problem.
@anntzer
Copy link
Contributor Author

anntzer commented Feb 27, 2020

Looks like in #16404 you put the note for the addition of the base kwarg in the wrong file, it should have gone to api_changes_3.2.0/behavior.rst, not next_api_changes/behavior.rst (which is now for 3.3). Can you fix that first?
@anntzer anntzer mentioned this pull request Feb 27, 2020
Closed
@tacaswell
Copy link
Member

tacaswell commented Mar 2, 2020

https://github.com/matplotlib/matplotlib/pull/16541/files I moved it in the backport but have not done the v3.2.x -> master merge up yet.
@greglucas greglucas mentioned this pull request Mar 3, 2020
Closed
4 of 9 tasks complete
@QuLogic
Copy link
Member

QuLogic commented May 1, 2020

@tacaswell preferred bumping the removal to 3.4 (#17242 (comment)), so re-milestoning this PR accordingly.
@QuLogic QuLogic added the Needs rebase label May 1, 2020
@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 1, 2020
@anntzer
Build lognorm/symlognorm from corresponding scales.
Loading status checks…
989b84f 
test_contour::test_contourf_log_extension has a tick move by one pixel,
but actually looks better with the patch?
@anntzer anntzer removed the Needs rebase label May 1, 2020
@anntzer anntzer force-pushed the anntzer:scale-norm branch from af8eb61 to 989b84f May 1, 2020
@dstansby
Copy link
Member

dstansby commented Jun 26, 2020

I'm going to try and fix-up symlognorm in #16376 - can I suggest that this PR just deals with LogNorm since we know that's mathematically correct at the moment? That way it will provide a good example of how to construct a norm from a scale, and I can handle symlognorm in one PR instead of having to track it over two different PRs.
@dstansby dstansby mentioned this pull request Jun 26, 2020
Open
@anntzer
Copy link
Contributor Author

anntzer commented Jun 27, 2020

I'm fine with that, but can you first check that the init signature wrangling (which is basically just there for SymLogNorm) works for you? I'm not sure I want to remove that part and then put it back.
@dstansby
Copy link
Member

dstansby commented Jun 29, 2020

Could you point out the bit of code you're referring to? I'm afraid I'm struggling to understand the new code here.
@anntzer
Copy link
Contributor Author

anntzer commented Jun 29, 2020

All the logic relating to init/init_signature is there to handle the fact that the signature of the norm and the scale don't match directly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak

Assignees

@jklymak jklymak

Labels
Needs review
Projects
None yet
Milestone
v3.4.0
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@anntzer @jklymak @tacaswell @QuLogic @dstansby
You can’t perform that action at this time.