SymLogNorm has a linear range that the docs stated was the same size as a decade in the logarithmic range. Except the logarithm being used in the code was base=np.e, not base=10, so it was not really a decade.

This PR prepares to change the default to 10 by adding abase kwarg, and warning if it is not specified. If a user wants to suppress the warning they should specifybase.

In 3.3. we may change the algorithm again slightly#16391, but will almost certainly default tobase=10 rather thannp.e.

This was discussed on the Feb 3 Dev call.

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

jklymak added this to thev3.2.0 milestone

Feb 4, 2020

jklymak added topic: color/color & colormaps Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labels

Feb 4, 2020

jklymak force-pushed thefix-add-base-symlognorm branch 2 times, most recently from8c1fee8 tofdb00b0Compare

February 4, 2020 14:52

tacaswell reviewed

Feb 10, 2020

View reviewed changes

lib/matplotlib/colors.py Outdated

		@@ -1213,7 +1213,7 @@ class SymLogNorm(Normalize):
		linthresh allows the user to specify the size of this range
		(-linthresh, linthresh).
		"""
		def __init__(self, linthresh, linscale=1.0,
		def __init__(self, linthresh, linscale=1.0, base=None,

Copy link

Member

tacaswellFeb 10, 2020

Choose a reason for hiding this comment

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

This needs to go at the end.

Copy link

MemberAuthor

jklymakFeb 10, 2020

Choose a reason for hiding this comment

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

Done...

Copy link

Member

KojoleyFeb 11, 2020

Choose a reason for hiding this comment

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

Is there a guide on which arguments we may/want to make keyword-only?

Copy link

MemberAuthor

jklymakFeb 11, 2020

Choose a reason for hiding this comment

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

Not really, but in this case, we can't really make any of then kwarg only because of back-compatibility concerns.

Copy link

Member

KojoleyFeb 11, 2020

Choose a reason for hiding this comment

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

I am talking only about the added one.

Copy link

Member

timhoffmFeb 15, 2020

Choose a reason for hiding this comment

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

As proposed in#16181, I‘d make almost all arguments kw-only. This will in particular also hold for added parameters.

Copy link

MemberAuthor

jklymakFeb 15, 2020

Choose a reason for hiding this comment

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

Sure, are you guys asking that just the new param is kwarg only, or all of them?

Copy link

Member

timhoffmFeb 16, 2020

Choose a reason for hiding this comment

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

For now, only the new parameter base can be kw-only. Making other parameters kw-only needs a deprecation first.

jklymak force-pushed thefix-add-base-symlognorm branch fromfdb00b0 toe16a257Compare

February 10, 2020 20:45

jklymak requested review fromdstansby andtacaswell

February 10, 2020 21:13

timhoffm reviewed

Feb 16, 2020

View reviewed changes

doc/api/next_api_changes/behaviour.rst OutdatedShow resolvedHide resolved

lib/matplotlib/colors.py OutdatedShow resolvedHide resolved

jklymakand others added3 commits

February 15, 2020 17:00

FIX: add base kwarg to symlognorm

c7df5d2

Update doc/api/next_api_changes/behaviour.rst

ec05b76

Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

Update lib/matplotlib/colors.py

fe5fdf8

Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

jklymak force-pushed thefix-add-base-symlognorm branch fromf6f3963 tofe5fdf8Compare

February 16, 2020 01:02

timhoffm approved these changes

Feb 16, 2020

View reviewed changes

Kojoley approved these changes

Feb 16, 2020

View reviewed changes

Kojoley merged commit3a8e75e intomatplotlib:master

Feb 16, 2020

Copy link

lumberbot-appbot commentedFeb 16, 2020

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v3.2.x$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 3a8e75ed151414ac15cbc0e6568f0d09fffdab31

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #16404: FIX: add base kwarg to symlognor'

Push to a named branch :

git push YOURFORK v3.2.x:auto-backport-of-pr-16404-on-v3.2.x

Create a PR against branch v3.2.x, I would have named this PR:

"Backport PR#16404 on branch v3.2.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free tosuggest an improvement.

lumberbot-appbot added the status: needs manual backport label

Feb 16, 2020

tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request

Feb 16, 2020

Merge pull requestmatplotlib#16404from jklymak/fix-add-base-symlognorm

2157863

FIX: add base kwarg to symlognorConflicts:doc/api/next_api_changes/behaviour.rst          - moved to            doc/api/prev_api_changes/api_changes_3.2.0/behavior.rst and re-worded.lib/matplotlib/colors.py          - implicitly backported converting the docstring to numpy            doc style.  Re-worded new docstring

tacaswell removed the status: needs manual backport label

Feb 16, 2020

jklymak added a commit that referenced this pull request

Feb 21, 2020

Merge pull request#16541from tacaswell/auto-backport-of-pr-16404-on…

bebc51a

…-v3.2.xMerge pull request#16404 from jklymak/fix-add-base-symlognorm

jklymak deleted the fix-add-base-symlognorm branch

February 26, 2020 03:27

jklymak mentioned this pull request

Feb 26, 2020

Build lognorm/symlognorm from corresponding scales.#16457

Merged

6 tasks

Copy link

Member

QuLogic commentedApr 25, 2020

The deprecation here is all confusing. It's marked as since 3.3, but 3.3 is when the deprecation is supposed to go away. The deprecation message also never had any version numbers to indicate when things changed or will change. I don't know if changing this after one minor release is best without this information.