BUG: fix a bug where 2D plots in spherical geometries would always use code-units in plot axes #3618

neutrinoceros · 2021-10-21T09:40:02Z

PR Summary

import yt

file = "KeplerianDisk/disk.out1.00000.athdf"
for us in ("cgs", "code"):
    ds = yt.load(file, unit_system=us)
    for normal in ("phi", "theta"):
        p = yt.SlicePlot(ds, normal, "density")
        p.plots["density"].figure.suptitle(f"unit system = {us}")
        p.save(f"/tmp/disk_slice_{us}.png")

gives

Marking this as draft because I ended up removing initialization code that was problematic in this specific case but I'd expect the tests to reveal why it was necessary.
In any cases, the code in question is duplicated in a lot of plot classes, so I'd need to apply consistent changes everywhere even if it turns out that it doesn't break anything.

neutrinoceros · 2021-10-21T09:48:32Z

yt/visualization/plot_modifications.py

@@ -2530,7 +2530,7 @@ def __call__(self, plot):
                un = self.time_unit.latex_representation()
                time_unit = r"$\ \ (" + un + r")$"
            except AttributeError as err:
-                if plot.ds.unit_system._code_flag == "code":
+                if plot.ds.unit_system._code_flag:


this isn't strictly necessary here but this looks like a different bug I stumbled upon while working here. The bug is that the _code_flag attribute is defined as a boolean, so this condition seems impossible to satisfy and probably dates back from an earlier implementation.

neutrinoceros · 2021-10-21T09:50:50Z

yt/visualization/plot_window.py

@@ -998,7 +998,7 @@ def _setup_plots(self):
            axis_index = self.data_source.axis

            xc, yc = self._setup_origin()
-            if self.ds.unit_system._code_flag or self.ds.no_cgs_equiv_length:
+            if self.ds.unit_system._code_flag:


I suspect this is a breaking change in other cases:
self.ds.no_cgs_equiv_length was apparently used as a proxy to detect some cartopy operations, but it happens to collide with how spherical dataset behave. Possibly the real bug is that this flag shouldn't be true for spherical datasets.

neutrinoceros · 2021-10-21T12:06:29Z

some cartopy tests failed, let's try again step by step

neutrinoceros · 2021-10-21T12:22:20Z

I think the bug dates back from #2378

neutrinoceros · 2021-10-21T12:34:31Z

so the current patch is still not satisfactory because it breaks units conversion for 'r-normal' plots where both axes are angular coordinates. It's starting to look like we need a replacement for the single no_cgs_equiv_length boolean, which seems to be too limited to work in every situations we want to support.
I believe the real issue is that we always store coordinates as lengths, even for angles, and this boolean is a workaround. Solving the deep underlying problem is just too much for me right now, so I'll try to develop somewhat more flexible workaround.

neutrinoceros · 2021-10-21T13:47:25Z

I think the current iteration is now stable in all cases I know of 🤞🏻
Note that I still expect one specific failure in an image comparison test that uses spherical coordinates because its current baseline contains the bug I'm trying to fix here

neutrinoceros · 2021-10-21T14:04:05Z

yt/data_objects/static_output.py


        self.unit_system = us
        self.unit_registry.unit_system = self.unit_system

+    @property
+    def _code_length(self):


I'm making this a property because self.no_cgs_equiv_length's value seems to be updated (True -> False) at some point during initialisation that I can not decipher

comment still relevant even though GH marks it as "outdated" because I've changed the name of the property

neutrinoceros · 2021-10-21T14:48:05Z

got exactly the "error" I expected/hoped for on Jenkins

expected (wrong)

actual (correct)

diff

…use code-units in plot axes

neutrinoceros · 2021-10-21T15:48:13Z

pinging for reviews:
@matthewturk as the author of #2378 that I'm partially reverting here
and @n-claes as the reporter for this bug

matthewturk · 2021-10-22T13:15:50Z

@neutrinoceros Do you have an example where code_units != cm ?

neutrinoceros · 2021-10-22T13:55:04Z

Unfortunately I don't. The only spherical data sets I know of are from the Athena++ and AMRVAC formats, none of which outputs unit metadata (I think ?) so both frontends default to cgs.

matthewturk · 2021-10-22T13:55:50Z

Could you force it by making them non-identical?

…

On Fri, Oct 22, 2021 at 8:55 AM Clément Robert ***@***.***> wrote: Unfortunately I don't. The only spherical data sets I know of are from the Athena++ and AMRVAC formats, none of which outputs unit metadata (I think ?) so both frontends default to cgs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3618 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO7TWPC5KDNMRPGGYEDUIFUMHANCNFSM5GNVPKCA> .

neutrinoceros · 2021-10-22T13:57:41Z

I don't know what you mean. What's identical to what ?

matthewturk · 2021-10-22T17:18:14Z

I'd like to see the difference before and after where it's also changing the values, not just the displayed units. If you can force this to happen by setting the unit system to be one where code_length does not correspond to cgs, that would be helpful.

neutrinoceros · 2021-10-22T17:33:48Z

I could just provide units to the AMRVAC frontend at load time, would that work ?

neutrinoceros · 2021-10-22T17:46:50Z

import yt
blast = "amrvac/bw_2d0000.dat"

units = ("cm", "AU")
for length_unit in units:
    us = {"length_unit": (1, length_unit)}
    ds = yt.load(blast, units_override=us)
    p = yt.SlicePlot(ds, "phi", "density")
    p.save(f"/tmp/slice_cgs_{length_unit}.png")

main branch

(x2)

this work

matthewturk · 2021-10-22T17:48:00Z

Thanks -- this satisfies me!

neutrinoceros · 2021-10-22T17:48:27Z

great, now let's bump this bad boy :)

neutrinoceros · 2021-10-22T17:51:50Z

@matthewturk I'm not sure you meant to do that: it seems like you set up auto merge but didn't approve

matthewturk · 2021-10-22T17:54:15Z

@neutrinoceros yeah, I like to withhold my approval

neutrinoceros · 2021-11-06T13:15:10Z

@meeseeksdev backport to yt-4.0.x

…al geometries would always use code-units in plot axes

…8-on-yt-4.0.x Backport PR #3618 on branch yt-4.0.x (BUG: fix a bug where 2D plots in spherical geometries would always use code-units in plot axes)

neutrinoceros added bug viz: 2D coordinates: non-cartesian labels Oct 21, 2021

neutrinoceros mentioned this pull request Oct 21, 2021

REL: yt 4.0.2 meta issue #3555

Closed

42 tasks

neutrinoceros force-pushed the hotfix_2913 branch from 0ae06f0 to 7ad6ea7 Compare October 21, 2021 09:46

neutrinoceros commented Oct 21, 2021

View reviewed changes

neutrinoceros force-pushed the hotfix_2913 branch from 7ad6ea7 to 8e0c1ea Compare October 21, 2021 12:06

neutrinoceros force-pushed the hotfix_2913 branch from 8e0c1ea to 1937bcd Compare October 21, 2021 12:23

neutrinoceros force-pushed the hotfix_2913 branch from 1937bcd to e8505cc Compare October 21, 2021 13:46

neutrinoceros commented Oct 21, 2021

View reviewed changes

neutrinoceros changed the title ~~BUG: fix a bug where sliceplots with spherical geometries would always use code-units in plot axes~~ BUG: fix a bug where 2D plots in spherical geometries would always use code-units in plot axes Oct 21, 2021

neutrinoceros force-pushed the hotfix_2913 branch from e8505cc to 4612717 Compare October 21, 2021 14:52

BUG: fix a bug where 2D plots with spherical geometries would always …

fda512b

…use code-units in plot axes

neutrinoceros force-pushed the hotfix_2913 branch from 4612717 to fda512b Compare October 21, 2021 14:53

neutrinoceros requested review from matthewturk and n-claes October 21, 2021 15:46

neutrinoceros marked this pull request as ready for review October 21, 2021 15:46

matthewturk enabled auto-merge October 22, 2021 17:48

bump answer for spherical slice plot

ddeb756

matthewturk approved these changes Oct 22, 2021

View reviewed changes

matthewturk merged commit 28c08fe into yt-project:main Oct 22, 2021

neutrinoceros deleted the hotfix_2913 branch October 22, 2021 20:10

meeseeksmachine pushed a commit to meeseeksmachine/yt that referenced this pull request Nov 6, 2021

Backport PR yt-project#3618: BUG: fix a bug where 2D plots in spheric…

6332315

…al geometries would always use code-units in plot axes

meeseeksmachine mentioned this pull request Nov 6, 2021

Backport PR #3618 on branch yt-4.0.x (BUG: fix a bug where 2D plots in spherical geometries would always use code-units in plot axes) #3648

Merged

This was referenced Mar 29, 2022

BUG: can't use annotate_timestamp with code units #3869

Closed

BUG: fix a regression where calling annotate_timestamp on a plot from a dataset with code units would crash #3870

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: fix a bug where 2D plots in spherical geometries would always use code-units in plot axes #3618

BUG: fix a bug where 2D plots in spherical geometries would always use code-units in plot axes #3618

neutrinoceros commented Oct 21, 2021 •

edited

Loading

neutrinoceros Oct 21, 2021

neutrinoceros Oct 21, 2021

neutrinoceros commented Oct 21, 2021

neutrinoceros commented Oct 21, 2021

neutrinoceros commented Oct 21, 2021 •

edited

Loading

neutrinoceros commented Oct 21, 2021 •

edited

Loading

neutrinoceros Oct 21, 2021

neutrinoceros Oct 22, 2021 •

edited

Loading

neutrinoceros commented Oct 21, 2021

neutrinoceros commented Oct 21, 2021

matthewturk commented Oct 22, 2021

neutrinoceros commented Oct 22, 2021

matthewturk commented Oct 22, 2021 via email

neutrinoceros commented Oct 22, 2021

matthewturk commented Oct 22, 2021

neutrinoceros commented Oct 22, 2021

neutrinoceros commented Oct 22, 2021

matthewturk commented Oct 22, 2021

neutrinoceros commented Oct 22, 2021

neutrinoceros commented Oct 22, 2021

matthewturk commented Oct 22, 2021

neutrinoceros commented Nov 6, 2021

BUG: fix a bug where 2D plots in spherical geometries would always use code-units in plot axes #3618

BUG: fix a bug where 2D plots in spherical geometries would always use code-units in plot axes #3618

Conversation

neutrinoceros commented Oct 21, 2021 • edited Loading

PR Summary

neutrinoceros Oct 21, 2021

Choose a reason for hiding this comment

neutrinoceros Oct 21, 2021

Choose a reason for hiding this comment

neutrinoceros commented Oct 21, 2021

neutrinoceros commented Oct 21, 2021

neutrinoceros commented Oct 21, 2021 • edited Loading

neutrinoceros commented Oct 21, 2021 • edited Loading

neutrinoceros Oct 21, 2021

Choose a reason for hiding this comment

neutrinoceros Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

neutrinoceros commented Oct 21, 2021

neutrinoceros commented Oct 21, 2021

matthewturk commented Oct 22, 2021

neutrinoceros commented Oct 22, 2021

matthewturk commented Oct 22, 2021 via email

neutrinoceros commented Oct 22, 2021

matthewturk commented Oct 22, 2021

neutrinoceros commented Oct 22, 2021

neutrinoceros commented Oct 22, 2021

matthewturk commented Oct 22, 2021

neutrinoceros commented Oct 22, 2021

neutrinoceros commented Oct 22, 2021

matthewturk commented Oct 22, 2021

neutrinoceros commented Nov 6, 2021

neutrinoceros commented Oct 21, 2021 •

edited

Loading

neutrinoceros commented Oct 21, 2021 •

edited

Loading

neutrinoceros commented Oct 21, 2021 •

edited

Loading

neutrinoceros Oct 22, 2021 •

edited

Loading