yt icon indicating copy to clipboard operation
yt copied to clipboard

Updating the linthresh guessing

Open chrishavlin opened this issue 2 years ago • 5 comments

This is an attempt at updating how the linthresh value is calculated for SymLogNorms when not provided. Note that I based it off of #3849 , so I'm submitting in draft mode until that one is cleared. The changes here are simple enough that it should be easy to update this PR after #3849 is fully reviewed.

IMO the linthresh calculation here nicely handles all of the concerns of #3944 without too much complexity. I need to update/add some tests and the docs would need a little updating, but it'd be great to get some feedback on the approach.

Conceptually, the calculation here uses the minimum absolute nonzero value as a starting guess at linthresh (as in #3295) but then limits the dynamic range of the symlog scale when necessary to avoid the floating point precision issues that caused the errors in #3858. The limiting is sensitive to the data so it does a decent job for a range of data distributions (and should not error for any).

Some images:

fn = 'FIRE_M12i_ref11/snapshot_600.hdf5'
ds = yt.load(fn)
p = yt.ProjectionPlot(ds, 'x', ('gas', 'density'), width=(30, 'Mpc'))
p.show()

image

And using the sample data from #3858

import yt

ds = yt.load('symlogsets/yt_bug_reproducer/plt04000')
slc = yt.SlicePlot(ds, "z", ("boxlib", "scalar"))
slc.show()

image

chrishavlin avatar Jun 24 '22 22:06 chrishavlin

Note that I experimented with the new imshow kwarg (interpolation_stage="rgba") but still got some rounding error crashes related to the normalization objects (SymLogNorm, LogNorm) and so gave up on that route...

chrishavlin avatar Jun 24 '22 22:06 chrishavlin

So the issue with using interpolation_stage is that it is only an imshow kwarg. When using the Norm objects directly, it's pretty easy to get it to error with the edge cases we're trying to handle here. e.g., using a linthresh close to the underflow limit will error:

from matplotlib.colors import SymLogNorm
v = SymLogNorm(1e-323, vmin=-1e-1, vmax=1e-1)
v(1e-2)  # errors with the familiar `ValueError: Invalid vmin or vmax`. 

There's some discussion of adding an interpolation stage argument to the Norm objects (e.g, the end of this issue), but even if that happens soon we'd then be handling multiple dependency conditions for the same underlying problem. And so I feel like it's a lot simpler to just try to avoid the conditions that give rise to the precision issues.

I mentioned this in one of my replies to a review comment here, but in addition to adding to the existing yt tests, I'm planning to do a more thorough matplotlib-dependent suite of tests to check all these edge cases before switching from draft.

chrishavlin avatar Jun 27 '22 15:06 chrishavlin

Seems reasonable, thank you for your research on this !

neutrinoceros avatar Jun 27 '22 17:06 neutrinoceros

Ok, so I rewrote all of _guess_linthresh. It felt clearer to avoid the whole abs(min,max) vs min(abs) confusion and treat the positive and negative ranges sequentially. It's maybe a little more verbose but I think it's easier to understand now.

chrishavlin avatar Jul 12 '22 21:07 chrishavlin

interesting... that failed test passed locally. Will have to dig in further...

chrishavlin avatar Jul 12 '22 22:07 chrishavlin

#3849 is in, now this should be rebased so we can see only the relevant bits in the diff

neutrinoceros avatar Aug 22 '22 17:08 neutrinoceros

I went for it since the rebase turned out a no op.

neutrinoceros avatar Aug 23 '22 20:08 neutrinoceros

I like this solution overall. The only thing I see missing from the PR is a brief mention of this in the colorbar linthresh narrative docs, like a sentence describing the linthresh guess (e.g., "linthresh uses the minimum absolute nonzero value but limits the dynamic range of the symlog scale when necessary to avoid floating point precision issues (10**15)"). Thanks for coding this up, Chris!

chummels avatar Aug 23 '22 21:08 chummels

Thanks for taking care of the rebase, @neutrinoceros ! I'll look through your comments ASAP.

chrishavlin avatar Aug 24 '22 15:08 chrishavlin

@chrishavlin there's a missing import reported by flake8

neutrinoceros avatar Aug 25 '22 15:08 neutrinoceros

Latest batch updates the docs, bumps the answers and incorporates the remaining suggestions. I'll go through and close out obviously outdated threads (done)...

chrishavlin avatar Aug 25 '22 16:08 chrishavlin