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

BUG: fix heuristic to switch from log scale to symlog scale #3793

Merged
merged 4 commits into from
Feb 17, 2022

Conversation

neutrinoceros
Copy link
Member

PR Summary

Fix #3791

Here's a simpler demo

import numpy as np
import yt

shape = (32, 16, 1)
a = np.linspace(0, 1, 16)
b = np.ones((32, 16))
c = np.reshape(a*b, shape)
data = {("gas", "density"): c}

ds = yt.load_uniform_grid(
    data,
    shape,
    bbox=np.array([[0.0, 5.0], [0, 1], [-0.1, +0.1]]),
)

p = yt.SlicePlot(ds, "z", "density")
p.save("/tmp/branch.png")

on main
main

this branch
branch

While the original issue was only about 2D plots, I'm also fixing a similar bug in line plots that I found on the way

@neutrinoceros neutrinoceros added this to the 4.0.3 milestone Feb 8, 2022
@neutrinoceros
Copy link
Member Author

Not sure what's happening in the two tests that failed. If I can't figure it out quickly I'll just remove this from the milestone

@neutrinoceros
Copy link
Member Author

Found that the failing tests were a consequence of #2517, and I discovered that the hack introduced back then isn't necessary after MPL 3.2 so I'm making it conditional. Now I'm expecting everything to stay green

@neutrinoceros neutrinoceros marked this pull request as draft February 8, 2022 18:26
@neutrinoceros
Copy link
Member Author

I've revised my approach to intentionally change some reference images. We have a couple tests (the one that have been failing) for which we plot some "binary" noise (i.e. all values are 0 or 1). The current gold standard looks like this
tmpkkshi4qk
which is not actually what we want: we see that all values plotted are equal (1), but we're masking zeros completely.
With the latest commit I'm getting this locally instead
3793_noise0_log_Slice_z_noise0
Now I'm waiting to see if my changes accidentally affect any other cases, and wether it makes them better or worse.

@neutrinoceros
Copy link
Member Author

This is now in the intented state. The only failing image tests are the ones I intend to update. I'll wait for an approval to bump them.

@neutrinoceros neutrinoceros marked this pull request as ready for review February 8, 2022 22:06
data[~np.isfinite(data)] = np.nan

zmin = float(self.zmin if self.zmin is not None else np.nanmin(data))
zmax = float(self.zmax if self.zmax is not None else np.nanmax(data))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the diff here is that we convert to float even in the second case np.nanmin... specifically to strip out units, so we can easily increment with +1 later on.

@neutrinoceros neutrinoceros marked this pull request as draft February 9, 2022 16:28
@neutrinoceros neutrinoceros force-pushed the hotfix_auto_symlog_min_0 branch 2 times, most recently from caa8ae0 to df34049 Compare February 9, 2022 16:33
@@ -298,7 +307,7 @@ def _init_image(self, data, cbnorm, cblinthresh, cmap, extent, aspect):
10
** np.arange(
np.rint(np.log10(cblinthresh)),
np.ceil(np.log10(zmax)),
np.ceil(np.log10(1.1 * zmax)),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is to intentionally overshoot zmax with the (excluded) upper boundary so that zmax can be used as a major tick location if it is itself an exact power of 10.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

)
)
)
if yticks[-1] > zmax:
yticks.pop()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

necessary cleanup with matplotlib 3.5 where having a ticks that's above the actual maximal value creates a white region at the top end of the colorbar (see #3554)

@neutrinoceros neutrinoceros marked this pull request as ready for review February 9, 2022 21:25
matthewturk
matthewturk previously approved these changes Feb 14, 2022
@@ -334,10 +343,12 @@ def _init_image(self, data, cbnorm, cblinthresh, cmap, extent, aspect):
10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I understand, would it be possible to do this with np.logspace? I do not think we should in this PR, for continuity sake, however.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it should be possible to refactor this with np.logspace. I also agree that it's preferable to keep diffs minimal for a bugfix PR

@neutrinoceros
Copy link
Member Author

Seems like the Jenkins job got killed because of network issues ?

hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@37a79109:JNLP4-connect connection from dxl1.ncsa.illinois.edu/141.142.193.70:39688": Remote call on JNLP4-connect connection from dxl1.ncsa.illinois.edu/141.142.193.70:39688 failed. The channel is closing down or has closed down

Let's try this again: @yt-fido test this please

@jzuhone jzuhone merged commit 60e9947 into yt-project:main Feb 17, 2022
@neutrinoceros neutrinoceros deleted the hotfix_auto_symlog_min_0 branch February 17, 2022 14:44
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.

BUG: a "hole" in cylindrical pixelization
3 participants