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

ENH: make plot callbacks with 'factor' argument accept 2-tuples to distinguish x/y resolutions #3686

Merged
merged 2 commits into from
Nov 24, 2021

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Nov 20, 2021

PR Summary

Fix #3685

import yt

ds = yt.load_sample("IsolatedGalaxy")

plot = yt.SlicePlot(ds, "z", "density", width=(0.04, 0.01))

plot.annotate_quiver(
    "velocity_x",
    "velocity_y",
    factor=(16, 64), # new !
    normalize=True,
    plot_args={"color": "white"},
)

plot.save("/tmp/factor.png")

factor

@neutrinoceros neutrinoceros added enhancement Making something better api-consistency naming conventions, code deduplication, informative error messages, code smells... viz: 2D and removed enhancement Making something better labels Nov 20, 2021
@neutrinoceros neutrinoceros force-pushed the enh_factor branch 2 times, most recently from ce48909 to a5d2e8f Compare November 20, 2021 18:15
@neutrinoceros neutrinoceros added the enhancement Making something better label Nov 20, 2021
@neutrinoceros neutrinoceros changed the title ENH: make plot callback with 'factor' argument accept 2-tuples to distinguish x/y resolutions ENH: make plot callbacks with 'factor' argument accept 2-tuples to distinguish x/y resolutions Nov 21, 2021
@neutrinoceros neutrinoceros force-pushed the enh_factor branch 4 times, most recently from aca83e7 to 807aede Compare November 21, 2021 20:32
Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

I think we're mostly OK here but I want to verify from @neutrinoceros that it's in the right x,y order, and ask why it has to be an integer.

if (
is_sequence(factor)
and len(factor) == 2
and all(isinstance(_, Integral) for _ in factor)
Copy link
Member

Choose a reason for hiding this comment

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

Why does it have to be an integer?

Comment on lines +965 to +964
nx = plot.image._A.shape[1] // self.factor[0]
ny = plot.image._A.shape[0] // self.factor[1]
Copy link
Member Author

Choose a reason for hiding this comment

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

@matthewturk this is why I'm checking for integers. While floats are mathematically compatible here, ultimately we're counting pixels, so I think that passing a float signals either a misconception on the user side or a bug

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I agree with that. We're specifically talking about the factor for changing the resolution; there is a substantial use case possible (even if not necessarily typically desirable) for values betweeen 0 and 1.

Copy link
Member Author

@neutrinoceros neutrinoceros Nov 22, 2021

Choose a reason for hiding this comment

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

would that work, really ? Running this on the main branch

import yt

ds = yt.load_sample("IsolatedGalaxy")

plot = yt.SlicePlot(ds, "z", "density", width=(0.04, 0.01))

plot.annotate_quiver(
    "velocity_x",
    "velocity_y",
    factor=0.5,
    normalize=True,
    plot_args={"color": "white"},
)

plot.save("/tmp/factor.png")

I'm getting

YTPlotCallbackError: annotate_quiver callback failed

Copy link
Member

Choose a reason for hiding this comment

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

Alright! Let's consider that addressed then.

@neutrinoceros
Copy link
Member Author

I want to verify from @neutrinoceros that it's in the right x,y order

Sorry I don't understand what I need to justify/explain here

@matthewturk
Copy link
Member

@neutrinoceros I mean, just say you checked that the order of x and y factors versus the image make sense, since we often do some strange transposition etc.

@neutrinoceros
Copy link
Member Author

It works in cases that I've tried, but I'm not sure how to test is extensively.

matthewturk
matthewturk previously approved these changes Nov 22, 2021
cphyc
cphyc previously approved these changes Nov 22, 2021
@neutrinoceros
Copy link
Member Author

Alright guys thank you for reviewing this early but I need to solve a conflict so I expect your approvals to go away after that. I'll remove the draft status when everything is green again :)

@neutrinoceros neutrinoceros marked this pull request as draft November 22, 2021 14:47
@neutrinoceros neutrinoceros dismissed stale reviews from cphyc and matthewturk via 72779ff November 22, 2021 14:48
@neutrinoceros
Copy link
Member Author

well actually we could also use auto-merge so no-one has to wait... removing the draft status now

@neutrinoceros neutrinoceros marked this pull request as ready for review November 22, 2021 14:49
cphyc
cphyc previously approved these changes Nov 22, 2021
@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@neutrinoceros
Copy link
Member Author

I pushed again to resolve a conflict. Auto merge is still enabled but I lost aprovals here. Could anyone approve (again) ?

@cphyc cphyc merged commit 2139a1c into yt-project:main Nov 24, 2021
@neutrinoceros neutrinoceros deleted the enh_factor branch November 24, 2021 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-consistency naming conventions, code deduplication, informative error messages, code smells... enhancement Making something better viz: 2D
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: missing flexibility plot callbacks with a factor argument
3 participants