Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Enh arbitrary scale#12818
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
Enh arbitrary scale#12818
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Note this is also a first-step in solving issues like#12665 and#12808 for colorbars with non-standard norms. Though we will have a bit of an issue getting from a norm to a transform. My guess is that the API here will need to expand to allow a Transform with an inverse as the argument as well as the two-tuple. But I'll let others comment on the basic idea first. |
lib/matplotlib/scale.py Outdated
| self._inverse = inverse | ||
| else: | ||
| raise ValueError('arguments to ArbitraryTransform must ' | ||
| 'be functions.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
generally, exception messages have no final dot (applies throughout)
lib/matplotlib/scale.py Outdated
| 'be functions.') | ||
| def transform_non_affine(self, values): | ||
| with np.errstate(divide='ignore', invalid='ignore'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think setting the errstate should be the job of the function itself? (aka. let's assume that whoever passes arbitrary functions in knows what they're doing, or document that they should)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sounds good to me. That was leftover from LogScale (I think), and I didn't quite grok what it was doing... I'll need to make an example that does the error checking though....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
well, in logscale it effectively belongs to the user-defined (meaning by us) function
lib/matplotlib/scale.py Outdated
| name = 'arbitrary' | ||
| def __init__(self, axis, functions=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just don't set a default tofunctions, as None isn't a valid value anyways?
Wonder whether the locator and formatter classes should be arguments too, i.e. I'd write something like
def __init__(self, axis, functions, *, major_locator_class=AutoLocator, minor_locator_class=NullLocator, major_formatter_class=ScalarFormatter, minor_formatter_class=NullFormatter):I think you're also going to run into "interesting" issues in shared-axis handling in the implementation of cla(), which assumes that you can "copy" a scale by doing
self.xaxis._scale = mscale.scale_factory( self._sharex.xaxis.get_scale(), self.xaxis)although it's not clear why the code doesn't just do
self.xaxis._scale = self._sharex.xaxis._scale...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thats a good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So the problem w/ passing the class instead of an instance of the class is that the Locators and Formatters need to be passed arguments, so I think it makes more sense to do
def __init__(self, axis, functions, *, major_locator=AutoLocator(), minor_locator=NullLocator(), major_formatter=ScalarFormatter(), minor_formatter=NullFormatter()):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But that won't work because you can't share a locator/formatter across multiple axes right now (as they keep a reference to their axis -- they probably shouldn't, but that's another in depth refactoring...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmmm, well, I guess folks will just have to change the locators and formatters manually
lib/matplotlib/scale.py Outdated
| TODO | ||
| """ | ||
| if functions is None or len(functions) < 2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd probably just writeforward, inverse = functions and get whatever error message you have when functions is not an iterable of len 2, rather than supplying your own error message. (After all, as of this commit passing a non-interable will also give you the builtin message.)
looks like you got an extra commit in there |
bf1154f tod403e65Compared403e65 to1e2d23eComparelib/matplotlib/scale.py Outdated
| Parameters | ||
| ---------- | ||
| forward: The forward function for the transform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be something like
forward : callable The forward function for the transform. It must have the signature:: def forward(values: array-like) -> array-likeAre there any additional constraints? E.g. must the function be monotonic? Do we need more precise type descriptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed to
forward: callable The forward function for the transform. This function must have an inverse and, for best behavior, be monotonic. It must have the signature:: def forward(values: array-like) -> array-likeThe forward function for the transform inverse: callable The inverse of the forward function. Signature as ``forward``.lib/matplotlib/scale.py Outdated
| axis: the axis for the scale | ||
| functions: (forward, inverse) | ||
| two-tuple of the forward and inverse functions for the scale. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SeeArbitraryTransform for details on the functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually this is more user facing so I changed this to have the more complete info as well. A bit repetitive, but...
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/scale.py Outdated
| @@ -545,6 +623,7 @@ def limit_range_for_scale(self, vmin, vmax, minpos): | |||
| 'log': LogScale, | |||
| 'symlog': SymmetricalLogScale, | |||
| 'logit': LogitScale, | |||
| 'arbitrary': ArbitraryScale, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's a bit misleading thatLogTransformBase can have anArbitraryScale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not following this comment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the suggestions@timhoffm This was the only one I didn't understand...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, I misread the code. Never mind.
Uh oh!
There was an error while loading.Please reload this page.
1e2d23e to8548e47CompareOnly did a quick read, but 👍 to the functionality. |
Minor preference for naming this |
OK< name changed to This needs to be squashed before merging because I have an extra image in there, but want to keep the old commit in case someone wants me to go back to |
| is_separable = True | ||
| has_inverse = True | ||
| def __init__(self, forward, inverse): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a reason why FuncTransform takes the two functions as separate args but FuncScale takes the pair as single arg? (Perhaps there is, didn't think more than that about it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just because the kwarg gets passed in as a tuple, whereas to me it makes more sense for the transform to take them separately. But I don't feel very strongly about it.
| axis.set_minor_formatter(NullFormatter()) | ||
| # update the minor locator for x and y axis based on rcParams | ||
| if rcParams['xtick.minor.visible']: | ||
| axis.set_minor_locator(AutoMinorLocator()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you incoorporate the fix from#12938 here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry for the delay - Done!
examples/scales/scales.py Outdated
| return np.rad2deg( | ||
| np.ma.log(np.abs(np.ma.tan(masked) + 1.0 / np.ma.cos(masked)))) | ||
| else: | ||
| return np.rad2deg(np.log(np.abs(np.tan(a) + 1.0 / np.cos(a)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you just rely on this returning nan for invalid values? (possibly wrap inwith np.errstate(invalid="ignore") to silence warnings)
At least the test below returns nan when square-rooting negative values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is just an example, right? More sophisticated users (than me) can add any error catching they want if they want to implement this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My point is that
# Function Mercator transformdef forward(a): a = np.deg2rad(a) return np.rad2deg(np.log(np.abs(np.tan(a) + 1 / np.cos(a))))is much shorter, and basically works just as well (AFAICT...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, probably. I just stole the code from thecustom_scale.py example to show that it was directly transferable. But I simplified in the latest commit.
5ce3637 to2ca2865Comparelib/matplotlib/tests/test_scale.py Outdated
| good = x >= 0 | ||
| y = np.full_like(x, np.NaN) | ||
| y[good] = x[good]**(1/2) | ||
| return y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess in the same vein something as simple as
def forward(x): return x**(1/2)also works (apparently it does in the scales.py example above...)? In which case you can probably even just pass them as lambdas below...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, they can be lambdas, but I don't find that easier to read than just defining the functions.... But main point taken...
My immediate thought on this is what happens if |
Garbage in, garbage out? |
2ca2865 to6220a18Comparelib/matplotlib/tests/test_scale.py Outdated
| @@ -1,6 +1,8 @@ | |||
| from matplotlib.testing.decorators import image_comparison | |||
| import matplotlib.pyplot as plt | |||
| from matplotlib.scale import Log10Transform, InvertedLog10Transform | |||
| import matplotlib.ticker as mticker | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the import is not actually needed?
lib/matplotlib/scale.py Outdated
| Both functions must have the signature:: | ||
| def forward(values: array-like) -> | ||
| array-likeThe forward function for the transform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
something wrong with the formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oooops. Should be
Both functions must have the signature:: def forward(values: array-like) returns array-likeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually
def forward(values: array-like) -> array-likelooks best to me (it's nearly normal type annotation syntax), but not insisting on that.
19a2475 tobb16ae4Comparelib/matplotlib/scale.py Outdated
| It must have the signature:: | ||
| def forward(values: array-like) -> | ||
| array-likeThe forward function for the transform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Goops, sorry, should have checked the whole thing
bb16ae4 tobc91f3eComparelib/matplotlib/scale.py Outdated
| def forward(values: array-like) -> array-like | ||
| inverse: callable | ||
| The inverse of the forward function. Signature as ``forward``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This paragraph is indented one character less than the one for forward.
Also the type annotations actually also use a spacebefore the colon (see rst syntax for definition lists, andnumpy/numpydoc#78).
Likewise for the docstring below.
API: add ability to set formatter and locators from scale initFIX: rename to FuncScaleDOC: add whats newFIX: simplify Mercator transformTST: simplify test
bc91f3e to9c74f77Compareping on this one - I think I've addressed all review suggestions.... Thanks! |
The 3.7 failure is spurious. |
Thanks a lot for reviewing and merging! |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
As pointed out by@ImportanceOfBeingErnest in the SecondaryAxes PR (#11859 (comment)), there is probably call for an arbitrary axis scale, so, here you go.
PR Checklist