Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Build lognorm/symlognorm from corresponding scales.#16457

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
QuLogic merged 1 commit intomatplotlib:masterfromanntzer:scale-norm
Jul 20, 2020

Conversation

anntzer
Copy link
Contributor

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 isFlake 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

return functools.partial(_make_norm_from_scale, scale_cls, init=init)

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

Choose a reason for hiding this comment

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

the signature overriding mess is because the signatures of SymLogScale and SymLogNorm are not consistent with one another either...

@jklymak
Copy link
Member

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.

@jklymakjklymak added this to thev3.3.0 milestoneFeb 10, 2020
@jklymakjklymak self-assigned thisFeb 24, 2020
@anntzer
Copy link
ContributorAuthor

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
Member

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
ContributorAuthor

Let's pick one strategy or the other before I actually do the work of rebasing? :)

@jklymak
Copy link
Member

#16404 is 3.2, so doing away w/ the deprecation should be fine for 3.3?

@anntzer
Copy link
ContributorAuthor

Are we fine with breaking after just one release?

@jklymak
Copy link
Member

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
Copy link
ContributorAuthor

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.

ba.apply_defaults()
super().__init__(
**{k: ba.arguments.pop(k) for k in ["vmin", "vmax", "clip"]})
self._scale = scale_cls(axis=None, **ba.arguments)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes.

@anntzer
Copy link
ContributorAuthor

Can you write the api break note? ;)

@jklymak
Copy link
Member

jklymak commentedFeb 27, 2020
edited
Loading

`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
Member

I think we still have a todo wrtSymLogNorm's documentation, but thats not really this PR's problem.

@anntzer
Copy link
ContributorAuthor

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?

@tacaswell
Copy link
Member

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.

@QuLogic
Copy link
Member

@tacaswell preferred bumping the removal to 3.4 (#17242 (comment)), so re-milestoning this PR accordingly.

tacaswell reacted with thumbs up emoji

@dstansby
Copy link
Member

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.

@anntzer
Copy link
ContributorAuthor

I'm fine with that, but can you first check that theinit signature wrangling (which is basically just there for SymLogNorm) works for you? I'm not sure I want to removethat part and then put it back.

@dstansby
Copy link
Member

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
ContributorAuthor

All the logic relating toinit/init_signature is there to handle the fact that the signature of the norm and the scale don't match directly.

@tacaswell
Copy link
Member

Anyone can merge this on the tests passing.

@jklymak is going to be the point of contact with@dstansby on a follow up PR.

test_contour::test_contourf_log_extension has a tick move by one pixel,but actually looks better with the patch?
@jklymak
Copy link
Member

@anntzer I need help using the new_make_norm_from_scale. We haveFuncScale, so makingFuncNorm seems easy enough. However, I can't see how to plug this into_make_norm_from_scale. If I just do

@_make_norm_from_scale(scale.FuncScale)class FuncNorm(Normalize):    """All inherited?"""

then
divnorm = colors.FuncNorm(functions=(forward, inverse))
then I get
TypeError: got an unexpected keyword argument 'functions'
despitefunctions being a kwarg ofFuncScale. So, how are we supposed to pass kwargs? You did it two ways in this PR, neither of which are terribly clear.

@jklymak
Copy link
Member

OK, I figured it out - you need to specify aninit(args) function. So for the above

@_make_norm_from_scale(scale.FuncScale,    init=lambda functions, vmin=None, vmax=None, clip=False: None)class FuncNorm(Normalize):    """All inherited?"""

works

@anntzer
Copy link
ContributorAuthor

anntzer commentedOct 4, 2020
edited
Loading

Just to be clear: This is because the order of parameters or even their default values is not the same between symlogscale and symlognorm, and there's no way to guess them (I could make assumptions about parameter order, e.g. everything goes before vmin/vmax/clip, but there's no way I could have autoguessed that for symlognorm, linthresh has no default whereas linscale defaults to 1 and base to 10; compare with symlogscale which has no default for any of them). So the solution I found was to just specify the defaults (and order) separately, viainit. (If I missed a better solution, I'm all ears :))

In reality it would be nice if the defaults of the scale and the norm could also be made consistent, but I sort of gave up on having any kind of consistency with symlog...

@jklymak
Copy link
Member

yes, but the way it is now, just doing it all with default doesn't work.init needs to be explicitly used so far as I could figure out. But I don't particularly understand all the signature binding etc.

@anntzer
Copy link
ContributorAuthor

Yes, init is needed. The way the norm is inited is basically "pretend you call theinit function with the parameters passed to the norm constructor, get the bound parameters (i.e. as if the body ofinit wasreturn locals() instead ofpass), and pass them to the scale constructor".

@jklymak
Copy link
Member

Yeah, I think the docstring for_make_norm_from_scale didn't get updated to reflect that very clearly. It makes it sound like you don't usually need theinit, but you really do regardless of whether the kwargs need to be reordered or not.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

@tacaswelltacaswelltacaswell approved these changes

@jklymakjklymakjklymak approved these changes

Assignees

@jklymakjklymak

Labels
None yet
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.

5 participants
@anntzer@jklymak@tacaswell@QuLogic@dstansby

[8]ページ先頭

©2009-2025 Movatter.jp