Skip to content

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 26, 2020

I didn't want to duplicate all the math explained in
SymmetricalLogScale, just to point out the (awkward) interaction between
transform and base/linthresh. (Probably one should deprecate
transform and make the Locator behave more like LogLocator...)

see https://discourse.matplotlib.org/t/the-transform-argument-of-symmetricalloglocator/20847/1 / https://gitter.im/matplotlib/matplotlib?at=5e2d39f71a1c2739e3002586

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

I didn't want to duplicate all the math explained in
SymmetricalLogScale, just to point out the (awkward) interaction between
*transform* and *base*/*linthresh*.  (Probably one should deprecate
*transform* and make the Locator behave more like LogLocator...)
@LaurenceMolloy
Copy link

LaurenceMolloy commented Jan 27, 2020

Looks like I also attempted to address the documentation gap as well. My bad!

I also note that the default parameter settings are not consistent between LogLocator and SymmetricalLogLocator. Perhaps they ought to be?

@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2020

Changing the defaults is always a pain due to backcompat concerns :(

@LaurenceMolloy
Copy link

Ordinarily, I wouldn't disagree with you, @anntzer. However, there's a special case here that may work in favour of changing defaults. SymmetricalLogLocator raises errors with the current None defaults, forcing the user to set either transform or both base and linthresh. No currently working code would actually be broken if base and linthresh were changed to match the LogLocator defaults as

  1. if user sets nothing, code will currently error
  2. If user sets transform, that takes precedence over base & linthresh values
  3. If user sets base and thresh, they override those defaults

However, perhaps not go as far as setting transform defaults as that would change the behaviour of existing user code that uses base and linthresh values.

You could also consider setting subs to match the default that is inferred within the body of the code so that it makes its default behaviour more visible to users.

At least this way, it's the same as LogLocator for the parameters that they share.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2020

Wouldn't it be simpler to just deprecate support for passing the transform kwarg (we have @cbook._delete_parameter for that), making the locator consistent with loglocator? (i.e. one would always need to pass the various relevant parameters -- base and linthresh -- separately).

@LaurenceMolloy
Copy link

LaurenceMolloy commented Jan 27, 2020

Ack that. The transform parameter does seem to have added complexity/confusion for no apparent purpose.

That said, would it be a bad thing to also set some non-null default values for base (10), linthresh (?) and subs ([1.0]) in SymmetricalLogLocator? SymmetricalLogLocator would then be instantiatable without any arguments, just like LogLocator.

This doesn't break any existing user code, as currently that has to contain overriding values for both base and linthresh for SymmetricalLogLocator to not raise ValueErrors upon instantiation. We would also only be setting subs default to the value that is set in the code (the same as for LogLocator).

These are simply my suggestions as someone new to the project codebase. I may be missing something important as I don't (yet) have the history with and knowledge of matplotlib that you do. Beyond deprecation of the transform parameter I'm happy to leave things as they are if you see no merit in any further amendment.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2020

That said, would it be a bad thing to also set some non-null default values for base (10), linthresh (?) and subs ([1.0]) in SymmetricalLogLocator? SymmetricalLogLocator would then be instantiatable without any arguments, just like LogLocator.

OK, I see your point now. Yes, this seems reasonable.

@QuLogic
Copy link
Member

QuLogic commented Mar 10, 2020

AFAICT, setting different defaults can be done in a separate PR.

@QuLogic QuLogic merged commit 32d3306 into matplotlib:master Mar 10, 2020
@QuLogic QuLogic added this to the v3.3.0 milestone Mar 10, 2020
@anntzer anntzer deleted the symlogdoc branch March 10, 2020 03:25
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.

3 participants