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 set_zlim method with symlog norm #3538

Merged
merged 1 commit into from
Oct 11, 2021

Conversation

neutrinoceros
Copy link
Member

PR Summary

fix #3229

import numpy as np
import yt

d = {'my_fake_density': np.random.random(size=(64,64,64)) - 0.5}

ds = yt.load_uniform_grid(d, [64,64,64])
p = yt.SlicePlot(ds, "z", ('stream', 'my_fake_density'))
p.set_log(('stream', 'my_fake_density'), True, linthresh=1.e-3)
p.set_zlim(('stream', 'my_fake_density'), -1e-3, 1e5)
p.save()

on main

zlim_symlog_main
This plot is identical before/after calling the set_zlim method.

this branch

zlim_symlog

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@neutrinoceros
Copy link
Member Author

requesting @chummels for review on this one :)

Copy link
Member

@chummels chummels left a comment

Choose a reason for hiding this comment

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

Thank you! I tested this out both with your example as well as my example from Issue #3229 , and they both behave correctly now. I just included some suggestions to remove extra debug print statements.

The only thing that I'll note is that the zmax doesn't appear to show up in as a tickmark/ticklabel in either of the examples. I imagine it could be fixed by slightly bumping the zmax by some epsilon value upward (zmax * 1.01). But other than that this is a great fix.

yt/visualization/base_plot_types.py Outdated Show resolved Hide resolved
yt/visualization/base_plot_types.py Outdated Show resolved Hide resolved
yt/visualization/base_plot_types.py Outdated Show resolved Hide resolved
@neutrinoceros neutrinoceros force-pushed the fixup_set_zlim_symlog branch from 60c3800 to 41ce5f5 Compare October 4, 2021 17:26
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Oct 4, 2021

The only thing that I'll note is that the zmax doesn't appear to show up in as a tickmark/ticklabel in either of the examples.

that sounds like a reasonable request, however, I anticipate that it'd be very hard to implement this in a way that would work consistently with every matplotlib versions we want to support, as they have been changing the way colorbars work a couple times between 3.0 and 3.5

@chummels
Copy link
Member

chummels commented Oct 4, 2021

But won't my option of just slightly increasing the zmax work regardless of the version? It seems like we just want to have the tick label, so making zmax exceed the tick marker value should work for any MPL version.

@chummels
Copy link
Member

chummels commented Oct 4, 2021

Maybe bumping down zmin by a similar amount would be wise too? Just to ensure it always has a tick label as well.

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Oct 4, 2021

I've seen similar tricks turn into maintenance puzzles with every recent matplotlib release. Matplotlib itself may be the place where this needs fixing. In any case I am very uncomfortable with introducing colorbar tricks in Yt when matplotlib 3.5 is coming up with a complete colorbar overhaul.

@matthewturk
Copy link
Member

I'm a little uncertain about removing the nanmax's in favor of straight max's, but I think it looks good.

@neutrinoceros
Copy link
Member Author

I'm a little uncertain about removing the nanmax's in favor of straight max's

I... don't think that's what I'm doing here. I'm actually keeping nanmaxes everywhere, aren't I ?

@matthewturk
Copy link
Member

I've marked this for merging once the conflicts are done.

@matthewturk matthewturk enabled auto-merge October 9, 2021 22:37
@chummels
Copy link
Member

chummels commented Oct 9, 2021

I agree with @neutrinoceros that there are no removed nanmaxes, as it's just defaulting to either nanmax or the value provided by the user (which will not be a nan).

@chummels
Copy link
Member

chummels commented Oct 9, 2021

I don't see what conflicts exist or need to be addressed before this gets merged.

@matthewturk
Copy link
Member

I don't see what conflicts exist or need to be addressed before this gets merged.

yt/visualization/base_plot_types.py

@neutrinoceros
Copy link
Member Author

fixed !

@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@matthewturk matthewturk merged commit 71f5772 into yt-project:main Oct 11, 2021
@neutrinoceros neutrinoceros deleted the fixup_set_zlim_symlog branch October 11, 2021 20:21
@neutrinoceros neutrinoceros mentioned this pull request Oct 12, 2021
42 tasks
@matthewturk
Copy link
Member

@meeseeksdev backport to yt-4.0.x

@lumberbot-app

This comment was marked as outdated.

@neutrinoceros
Copy link
Member Author

@meeseeksdev backport to yt-4.0.x

@lumberbot-app

This comment was marked as outdated.

@neutrinoceros
Copy link
Member Author

Let's try this one last time now that a lot of other PRs went in. If it fails we'll resort to manual backport
@meeseeksdev backport to yt-4.0.x

@lumberbot-app
Copy link

lumberbot-app bot commented Nov 6, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout yt-4.0.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 71f5772b7f147f21acff614198fd8f12d79390c1
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #3538: BUG: fix set_zlim method with symlog norm'
  1. Push to a named branch :
git push YOURFORK yt-4.0.x:auto-backport-of-pr-3538-on-yt-4.0.x
  1. Create a PR against branch yt-4.0.x, I would have named this PR:

"Backport PR #3538 on branch yt-4.0.x (BUG: fix set_zlim method with symlog norm)"

And apply the correct labels and milestones.

Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove Still Needs Manual Backport label once the PR gets merged.

If these instruction are inaccurate, feel free to suggest an improvement.

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.

set_zlim does not work for symlog scaling
3 participants