Skip to content

PiecewiseLinearNorm #5061

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

Closed

Conversation

OceanWolf
Copy link
Member

Working implementation of a PiecewiseLinearNorm, including a DivergingNorm subclass.

All the tests from the @dopplershift and @phobson at present pass apart from the tests that have norms that don't monotonically increase (i.e. they stay static) e.g. where vmin=vcenter and/or vcenter=vmax.

Should we allow this or not?

phobson and others added 16 commits September 14, 2015 00:42
Borrows heavily from @Tillsen's solution found on
StackOverflow here: http://goo.gl/RPXMYB

Used with his permission dicussesd on Github here:
https://github.com/matplotlib/matplotlib/pull/3858`
This will allow the ticks of colors to be spaced as desired.
Also simplified the math per the brilliant @joferkington
http://stackoverflow.com/a/20146989/1552748
couldn't actually run the test suite b/c python
iesn't installed as a framework.
Need to allow it in __call__ since colorbar can pass a value for clip.
To make it more similar to the other norms

Also remove misleading comment about returning scalars
Since possibly some earlier versions of numpy returned a scalar,
wrap the value in atleast_1d before indexing.
np.interp handles the case vmin == vcenter, we have to add a special case
to make the last test case pass
@OceanWolf
Copy link
Member Author

Something that strikes me here, but I don't think for now, how the class inheritance goes

  1. object
  2. Normalise (2 stops min/max)
  3. PiecewiseLinearNorm (generic n stops)
  4. DivergingNorm (3 stops, min/center/max)

I.e. it should go from the most general at the end of the MRO to most specific at the top... instead we have the most specific, Normalise at the bottom... not something I specifically want to address now, both because I know I have a lot of more fundamental refactoring to do in this area, and I want to make sure I get that right before fiddling about here; and also because having slept on it, I would like to see the OrderedDict on the MRO, rather than existing as just an attribute, and that will have to wait until we drop python-2.6

@efiring
Copy link
Member

efiring commented Sep 14, 2015

@OceanWolf Normalize is not the most specific, it is the simplest and most common. It is no more ore less specific than any of its subclasses. Yes, one could split out a NormalizeBase and use that as the common ancestor for all. Is the gain in formal structure worth the additional LOC and the extra level of redirection for the Normalize case, which is probably something like 95% of real world usage?

@OceanWolf
Copy link
Member Author

@efiring by most specific I mean that as far as I can tell Normalize does the same thing as DivergingNorm but with vcenter = np.mean([vmin, vmax]).

DivergingNorm does the same thing as PiecewiseLinearNorm but with just one stop between vmin and vmax set specifically at the centre.

I don't see what you mean by "additional LOC", as far as I see it we significantly reduce the LOC, i.e. the Normalise class becomes something like:

class Normalize(DivergingNorm):
  def __init__(self, vmin=None, vmax=None):
    super(Normalize, self).__init__(vmin, None, vmax)

@Tillsten
Copy link
Contributor

Btw the is a lot of overlap between transforms and the norms, it would be nice - a properly quite difficult - to combine these.

@OceanWolf
Copy link
Member Author

@Tillsten Yes, I have plans to do this with my MEP working on refactoring the axes #5029, I plan to merge the colourbar with the other axis code, essentially these Normalisations works as axis transformations, so it makes sense to me to simply the API here.

As these extra norms have been pushed back to at least the 2.1 release next April, this gives me time to work on this MEP. We shall see how far I get with this. Help always appreciated, especially as I have MEP 22 and 27 to finish for 2.1 once 1.5 lands.

@tacaswell tacaswell added this to the proposed next point release (2.1) milestone Oct 8, 2015
@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Sep 24, 2017
@jklymak
Copy link
Member

jklymak commented Oct 6, 2018

See #12419 where I moved this PR (couldn't see force pushing here as viable)

@jklymak jklymak closed this Oct 6, 2018
@story645 story645 removed this from the future releases milestone Oct 6, 2022
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.

9 participants