BUG: fixes for Hammer-Aitoff projection (r-normal) 2D viz in spherical geometries

[neutrinoceros]

PR Summary

This is a jab at the first item in #1796
Spherical geometries need more work but this is a first step to improve maintainability.
Right now, the defintions of "theta" and "phi" are misaligned between the aitoff pixelizer and the rest of the code base.
Fortunately, the latter is correct (aligned with most common notations AFAIC), while the former isn't user facing so it's an easy fix.

edit: this now completes the 3rd item of #1796

[neutrinoceros]

Also fixes a bug that I unfortunately didn't see in #3532: x and y axis labels in r-normal plots were not only incorrectly typeset, they were also exchanged.
edit: actually given that these plot are done using the Hammer-Aitoff projection, the labels are in fact completely wrong, as they do not map to angular coordinates at all. I'm getting closer and closer to a solution for #3610

[neutrinoceros]

So while I'm at it I figured I might as well give a shot at fixing #3610 and made some progress there
rerunning the demo script I wrote in a comment (#3610 (comment))

import yt

ds = yt.load_sample("KeplerianDisk")
p = yt.SlicePlot(ds, "r", "density")
p.save(f"/tmp/disk_slice_r.png", mpl_kwargs=dict(bbox_inches="tight"))

I'm now getting this:

There are still a couple issues, but I was able to

• fix axes labels
• ... including units (none ! that's a new special case)
• compute the edges and center of the image correctly

remaining issues:

1. whitespace on the top and bottom of the image should be much thiner. This is a bug in the pixelizer itself that I think was coded with only full spherical domains in mind.
2. the colormap label incorrectly uses code-units (sigh... this bug is never going away, is it ?)

So I still want to fix 1) here, but I may reserve 2) for a different PR

[neutrinoceros]

Update:

• fixed labels again (introduce a different notation to distinguish latitude (theta') from colatitude (theta)), same with longitude and azimuth. This will need to be better documented (for now I'm commenting the code)
• fixed the figure space's occupancy, though obviously the pixelizer needs more work still

I think I'm getting there, but I need to carefully read the pixelizer code to finish my generalisation

[matthewturk]

@neutrinoceros could you expand on the "obviously"? I'm not sure I understand specifically what you mean.

[neutrinoceros]

Let me rephrase this more carefully:
I think I understand everything that happens outside the pixelizer routine and that I'm currently assuming that it's the only place left where a bug could lurk.

I've been simplifying the math locally (not pushed yet) but so far I'm still stuck with the visual offset shown above

[neutrinoceros]

Currently the routine is hitting the right amount of pixels and the end product it very close to what I expect, but something about coordinate transformations seems to be off. Any help / insight would be greatly appreciated, I haven't been able to make any significant progress for a couple hours now.

[neutrinoceros]

Now we're getting somewhere

I think I'm done with the bugs I wanted to deal with for now. I'll come back to see if I need to fix anything after CI completes. Otherwise I'll just clean up the branch and rebase before I open this to review.

[neutrinoceros]

So while this fixes the viz for the Athena++ "KeplerianDisk" dataset, it also ruins it in the case of the AMRVAC "bw_spherical_2d" dataset (gives out a blank image). As noted in #3610, a potentially key difference between the two formats is in the axis order, see #3610 (comment)

[matthewturk]

I think this is definitely the more correct approach, but I am vaguely left wishing we had a better way to display so we could have something more terse.

…

On Fri, Oct 29, 2021 at 4:11 PM Clément Robert ***@***.***> wrote: So while I'm at it I figured I might as well give a shot at fixing #3610 <#3610> and made some progress there rerunning the demo script I wrote in a comment (#3610 (comment) <#3610 (comment)>) import yt ds = yt.load_sample("KeplerianDisk")p = yt.SlicePlot(ds, "r", "density")p.save(f"/tmp/disk_slice_r.png") I'm now getting this: [image: disk_slice_r] <https://user-images.githubusercontent.com/14075922/139501566-a452a727-1a9c-4cb8-b831-d9f7e60f0cdc.png> There are still a couple issues, but I was able to - fix axes labels - ... including units (none ! that's a new special case) - compute the edges and center of the image correctly remaining issues: 1. whitespace on the top and bottom of the image should be much thiner. This is a bug in the pixelizer itself that I think was coded with only full spherical domains in mind. 2. the colormap label incorrectly uses code-units (sigh... this bug is never going away, is it ?) So I still want to fix 1) here, but I may reserve 2) for a different PR — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3628 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXOZC5EE6GQTKLN6ZOSTUJML2PANCNFSM5G7WCJRA> .

[neutrinoceros]

Me too, but there's too much going on, so I'm focusing on making it work for now.
The bounds are not computed correctly for the AMRVAC dataset so I'll try to fix this later today.

[neutrinoceros]

There was a confusion in my definition of the latitude I wrote that
latitude = colatitude - PI/2

but it's really
latitude = PI/2 - colatitude

As a result the pixelizer is likely inverting North and South. In fact I know it does: here's a field that's is positive in the North half and negative in the South half:

(this is an Idefix dataset, I can't easily share the reproducer for now)

[neutrinoceros]

fixed

[neutrinoceros]

Update:

• I changed the notation in the labels again (I really need to document them but I think the ones I got now are the best choice I can find)
• the reason why this is still a draft is because I know a case that completely breaks with this patch/

import yt

ds = yt.load_sample("bw_spherical_2d", unit_system="code")
# using the aspect argument only to limit the height of the second image
# improvement is underway in https://github.com/yt-project/yt/pull/3633
p = yt.SlicePlot(ds, "r", "density", aspect=0.5)
p.save(f"/tmp/test.png")

main branch:

this work (current):

so I think what's happening is an unfortunate side effect of the default "origin" argument in SlicePlot which doesn't plays well here because we can't simply translate the frame as we do in cartesian. I haven't looked into how to test hunch this properly, but I know that the bounds are correctly computed in SphericalCoordinateHandler.
@matthewturk , any inputs here ?

[neutrinoceros]

Fixed it !

Now I'll wait one more round of CI to check I have not broken anything and then I'll rebase + open for review

[neutrinoceros]

The one failing test is checking reproducibility of the FRB for basic pixelization calls. It can be visually reproduced with

import yt

ds = yt.testing.fake_amr_ds(geometry="spherical")
p = yt.SlicePlot(ds, "r", "Density")
p.save("/tmp/")

main branch

this branch

So the result is indeed different, but I believe that it's more correct with this branch: Aitoff coordinates range from -1 to +1 in both directions, so with the default aspect ratio (1), projecting a complete sphere should produce a disk, not an ellipse.
I'll request authorisation to bump this answer test.

[neutrinoceros]

I found some more inconsistencies, this is getting back to the shop.

[neutrinoceros]

@matthewturk can I bump answers here ? it's fine if you want to have a deeper look at the code first, just please let me know

[matthewturk]

do it!

…

On Fri, Nov 19, 2021 at 12:55 PM Clément Robert ***@***.***> wrote: @matthewturk <https://github.com/matthewturk> can I bump answers here ? it's fine if you want to have a deeper look at the code first, just please let me know — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3628 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO74YHU3NKTWNZKLB6LUM2MR7ANCNFSM5G7WCJRA> .

[neutrinoceros]

Ah, the report is gone, so I'll rebase first and bump tomorrow. Switching to draft again.

[matthewturk]

The changes inside the pixelizer (specifically reversing the order, etc) are a bit confusing to me, but I see that the resultant images are OK, so I don't object.

[matthewturk]

I'm pretty ambivalent about the idea of changing all the notation in here, but I suppose it does make things clearer.

[matthewturk]

Can you spare a moment to explain why this is different now? It's the reverse of before, yes?

[neutrinoceros]

yes. AFAICT, everywhere in the code base, we denote the colatitude "theta". As the name suggests, colatitude is the complementary angle to latitude (so lat + coat = PI/2). On the main branch, the angle that's noted phi here effectively is the opposite of latitude. So I'm changing the name to btheta to reflect that it's neither the azimuth (phi) nor the colatitude (theta), and I'm changing its sign because... that's just a bug (I discovered it in testing when I saw a sphere being displayed upside down, with the north emisphere on the bottom half).

[neutrinoceros]

Something went terribly wrong in the docs build but I don't believe it's reproducible
@yt-fido test this please

[neutrinoceros]

Read too fast, this is perfectly reproducible indeed, but I'm still surprised that we're only seeing this now

AttributeError: 'TitleCallback' object has no attribute '_supported_geometries'

[neutrinoceros]

This error is absolutely mysterious to me. TittleCallback instances should always inherit a _supported_geometries from the base PlotCallback class. Also worth mentioning CI was green on c7304b4 but went red with 110185c with a clearly unrelated error.
I'm lost. @Xarthisius, any idea what might be going on ?

[Xarthisius]

I'm lost. @Xarthisius, any idea what might be going on ?

Have you tried to reproduce it locally with current main? Usually that happens when a PR that didn't trigger docs build gets merged and breakage goes unnoticed.

[neutrinoceros]

I did try

cd doc
python helper_scripts/run_recipes.py

but this seems even more broken on my machine (I get stuck in a loop of error messages where processes fail to spawn because of some race condition ? and it didn't get better after I required a single process instead of 6 or replaced Pool.async_map with Pool.map)

[Xarthisius]

I did try

cd doc
python helper_scripts/run_recipes.py

Next time, don't try to run everything. Run the thing that fails. E.g. in the traceback I posted (#3690 (comment)) it sufficient to do:

python doc/source/visualizing/colormaps/cmap_images.py

to easily hit that error.

[neutrinoceros]

Thank you, dully noted.
In the mean time I was able to find the bug (which is indeed on the main branch). Posting a PR in a couple minutes

[neutrinoceros]

@yt-fido test this please

[neutrinoceros]

@matthewturk should this go in ?