Skip to content
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

Set norms using scale names. #20752

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Set norms using scale names. #20752

wants to merge 1 commit into from

Conversation

@anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 27, 2021

PR Summary

Proof of concept implementation for #20746.
Still needs a lot of docs, but otherwise mostly works.

Remaining issues:

  • autoscale() will run into issues for scales with limited domains, e.g. we should/need to be able to auto-infer that LogNorm only normalizes based on the positive values (cf. overrides of autoscale() in LogNorm).
  • The autogenerated norm classes are not picklable, which can be considered a slightly obscure limitation, but can be solved using semi-standard boilerplate as in #19033. This is tracked by #20755.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
colors.Normalize)()
norm_cls = type(norm)
norm_cls.__name__ = f"{scale_cls.__name__}Norm"
return norm_cls

This comment has been minimized.

@jklymak

jklymak Jul 28, 2021
Contributor

I guess this seems overkill to me, and fragile as shown by the pickling issue? Why don't we just use scale._scale_mapping or make one, i.e. colors._norm_mapping? Do we really need the extra flexibility given by allowing dynamic creation?

This comment has been minimized.

@anntzer

anntzer Jul 28, 2021
Author Contributor

I don't really want to create a separate mapping as people can register new scales (e.g. mpl-probscale does it), but they wouldn't show up in the norm mapping if these are separate.

This comment has been minimized.

@jklymak

jklymak Jul 28, 2021
Contributor

Back when I was a whippersnapper, we only had the linear norm and we liked it....

I'm not sure that making this super-flexible is worth the complication. The strings can be for a few common norms. How many specialized scales truly need a norm as well, and how much of a problem is it to ask users to just pass the norm instead to the string? Its not like probscale is going to be a super-useful color norm. If you wanted to add a register_norm, fine, but making it all dynamic just seems to be asking for trouble.

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

Successfully merging this pull request may close these issues.

None yet

2 participants