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: Order of Cylindrical Coordinates for Athena++ Incorrect #4796

Open
forrestglines opened this issue Jan 31, 2024 · 6 comments
Open

BUG: Order of Cylindrical Coordinates for Athena++ Incorrect #4796

forrestglines opened this issue Jan 31, 2024 · 6 comments
Labels
bug code frontends Things related to specific frontends coordinates: non-cartesian

Comments

@forrestglines
Copy link
Contributor

Bug report

Bug summary

The order of coordinates for cylindrical datasets from Athena++ seem to be incorrect. I ran across this issue writing #4323

I have a hot-fix for just my purposes to reorder the coordinates. However, changing the geometry to Geometry.POLAR might be a better fix once #4790 is done. I can provide my hot-fix on request.

Code for reproduction

Here's the athdf file for a 2D Keplerian disk with uniform r-coordinates modified from the input deck athinput.disk_cyl included with Athena++:

I try to plot the disk with

import yt
ds = yt.load("disk.out1.00000.athdf")
slc = yt.SlicePlot(ds, "z", ("gas", "density"))
slc.show()

Actual outcome

main-disk out1 00000_Slice_z_density

Expected outcome

Whereas if I force the correct coordinate ordering by changing the Athena++ frontend to respect $(r,\theta,z)$ I get the expected disk

fix-disk out1 00000_Slice_z_density

Version Information

Using current main commit: d5b3335

@forrestglines forrestglines changed the title BUG: Order of Cylindrical Coordinates Athena++ Incorrect BUG: Order of Cylindrical Coordinates for Athena++ Incorrect Jan 31, 2024
@neutrinoceros
Copy link
Member

Hi @forrestglines, thank you for reporting. I think this is reasonably decoupled with #4790 so feel free to open a PR with your fix at any point !

@neutrinoceros neutrinoceros added bug code frontends Things related to specific frontends coordinates: non-cartesian labels Jan 31, 2024
@matthewturk
Copy link
Member

It might be worth checking that we're using the ds.coordinates.axis_name dict wherever we need to; that's a potential source of problems in the selection and pixelization routines. (i.e., assuming that 0 == r, 1 == theta, 2 == phi, rather than using the axis_names to map)

@forrestglines
Copy link
Contributor Author

Hi @forrestglines, thank you for reporting. I think this is reasonably decoupled with #4790 so feel free to open a PR with your fix at any point !

Thanks for the input! This issue with coordinates is the same issue I encountered developing the Parthenon backend. The two data formats (and the AthenaPK code) are extremely similar so I'd like the frontends to behave similarly. I see two options preferable options to fix both:

  1. We go with Geometry.CYLINDRICAL and change axis_order for both Athena++, as I do in Fix order of Athena++ cylindrical coords #4801, and for Parthenon as I originally did in Add Parthenon frontend #4323

  2. We make both Athena++ and Parthenon use Geometry.POLAR and we fix BUG: fix FRB integration with CylindricalResolutionBuffer #4790.

I'm a bit inclined to go with the first option to keep Athena++ as Geometry.CYLINDRICAL and have Parthenon match that solution. I'll still help out to debug #4790.

@forrestglines
Copy link
Contributor Author

It might be worth checking that we're using the ds.coordinates.axis_name dict wherever we need to; that's a potential source of problems in the selection and pixelization routines. (i.e., assuming that 0 == r, 1 == theta, 2 == phi, rather than using the axis_names to map)

Thanks @matthewturk! This is a suggestion for implementation for the selection and pixelization routines and what's likely causing issues in #4790, right? In this issue I'm having the trouble that yt by default uses r == 0, z == 1, theta == 2 for cylindrical dataset when Athena++ cylindrical datasets are r == 0, theta == 1, z == 2.

Does the fix in #4801 look congruent with yt standards/expectations? It only changes the assumptions about Athena++ cylindrical dataset but hopefully no other assumptions in the code.

@neutrinoceros
Copy link
Member

I see two options to fix both:

I don't think those options should be mutually exclusive: if using Geometry.CYLINDRICAL works for you and avoids the current bugs with Geometry.POLAR, that's a reasonable thing to do as a workaround, and we can always switch both frontends to use the more correct solution (POLAR) once known issues are addressed. What do you think ?

@forrestglines
Copy link
Contributor Author

I don't think those options should be mutually exclusive: if using Geometry.CYLINDRICAL works for you and avoids the current bugs with Geometry.POLAR, that's a reasonable thing to do as a workaround, and we can always switch both frontends to use the more correct solution (POLAR) once known issues are addressed. What do you think ?

I think this is the right solution, this is good enough for both frontends. When the issues with POLAR are addressed we can switch both frontends.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug code frontends Things related to specific frontends coordinates: non-cartesian
Projects
None yet
Development

No branches or pull requests

3 participants