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: Add millimeter to smallest units #4387

Merged
merged 3 commits into from
Apr 6, 2023

Conversation

matthewturk
Copy link
Member

This is a very small fix. For datasets that are in units of centimeters, any
zooming in on plots will switch from 1cm to 1e3 um in the base units of the
image axes.

This minor change inserts mm as an option for switching, which changes it to
allow for a middle-ground. Seconds already allows ms so I think this should
also be fine.

@neutrinoceros
Copy link
Member

I'm tempted to triage this as a bugfix since the previous behaviour was likely never intended. I've seen this problem before, but do you have a reprod that we can come back for posterity ?

@matthewturk
Copy link
Member Author

Sure, here's an example:

ds = yt.load_sample("SecondOrderTets", step=-1)
s = yt.SlicePlot(ds, "z", ("all", "ux"), center=[0.0, 0.0, 3.0])
s.annotate_mesh_lines(color = 'k')
s.pan((0.0, -2.0))
s.zoom(5)
s.show()

@matthewturk
Copy link
Member Author

Oops, forgot to attach, but here's current output.

image

neutrinoceros
neutrinoceros previously approved these changes Mar 30, 2023
@neutrinoceros neutrinoceros enabled auto-merge March 30, 2023 14:25
@neutrinoceros
Copy link
Member

woops, one real failure, let's turn auto-merge off

@neutrinoceros neutrinoceros disabled auto-merge March 30, 2023 15:07
@neutrinoceros
Copy link
Member

I think the failing test should be modified. It seems to be too tightly coupled with implementation details
This is how I'd fix it

diff --git a/yt/visualization/tests/test_fits_image.py b/yt/visualization/tests/test_fits_image.py
index 147b8abbf..26cc26a48 100644
--- a/yt/visualization/tests/test_fits_image.py
+++ b/yt/visualization/tests/test_fits_image.py
@@ -100,8 +100,8 @@ def test_fits_image():
 
     assert_equal(fits_slc2["density"].data, fits_slc["density"].data)
     assert_equal(fits_slc2["temperature"].data, fits_slc["temperature"].data)
-    assert fits_slc.wcs.wcs.crval[0] == ds.domain_center[0].to_value("cm")
-    assert fits_slc.wcs.wcs.crval[1] == ds.domain_center[1].to_value("cm")
+    assert fits_slc.wcs.wcs.crval[0] != 0.0
+    assert fits_slc.wcs.wcs.crval[1] != 0.0
     assert fits_slc2.wcs.wcs.crval[0] == 0.0
     assert fits_slc2.wcs.wcs.crval[1] == 0.0

Though if you're worried about loosened comparison, here's the patch that would preserve the current spirit of the test

diff --git a/yt/visualization/tests/test_fits_image.py b/yt/visualization/tests/test_fits_image.py
index 147b8abbf..26cc26a48 100644
--- a/yt/visualization/tests/test_fits_image.py
+++ b/yt/visualization/tests/test_fits_image.py
@@ -100,8 +100,8 @@ def test_fits_image():
 
     assert_equal(fits_slc2["density"].data, fits_slc["density"].data)
     assert_equal(fits_slc2["temperature"].data, fits_slc["temperature"].data)
-    assert fits_slc.wcs.wcs.crval[0] == ds.domain_center[0].to_value("cm")
-    assert fits_slc.wcs.wcs.crval[1] == ds.domain_center[1].to_value("cm")
+    assert fits_slc.wcs.wcs.crval[0] == ds.domain_center[0].to_value("m")
+    assert fits_slc.wcs.wcs.crval[1] == ds.domain_center[1].to_value("m")
     assert fits_slc2.wcs.wcs.crval[0] == 0.0
     assert fits_slc2.wcs.wcs.crval[1] == 0.0

@neutrinoceros neutrinoceros merged commit bf56dbc into yt-project:main Apr 6, 2023
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.

2 participants