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

[WIP] ENH: Cartesian fields in spherical geometry #4497

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matthewturk
Copy link
Member

PR Summary

This adds some cartesian fields (cartesian_x etc) to the spherical coordinate handler.

Here is a script that plots what these look like:

import yt
import yt.testing

ds = yt.testing.fake_amr_ds(geometry="spherical")

c = ds.domain_center

ds.r[c[0],:,:].plot("cartesian_x").set_log("all", False).save()
ds.r[c[0],:,:].plot("cartesian_y").set_log("all", False).save()
ds.r[c[0],:,:].plot("cartesian_z").set_log("all", False).save()

ds.r[:,c[1],:].plot("cartesian_x").set_log("all", False).save()
ds.r[:,c[1],:].plot("cartesian_y").set_log("all", False).save()
ds.r[:,c[1],:].plot("cartesian_z").set_log("all", False).save()

ds.r[:,:,c[2]].plot("cartesian_x").set_log("all", False).save()
ds.r[:,:,c[2]].plot("cartesian_y").set_log("all", False).save()
ds.r[:,:,c[2]].plot("cartesian_z").set_log("all", False).save()

The images that result (which I will attach momentarily to this PR) look correct, I think, except where the AMR regions interact. In those areas, I'm much less certain, as it's possible that they are correct because we are doing cell-centering and the slicing cuts oddly, but I'm genuinely not sure because they do also look out of place. It would be very helpful to have some other thoughts on that.

@matthewturk
Copy link
Member Author

Slices along phi (i.e., a half-circle that runs along the z axis) of the three coordinate values:

AMRGridData_Slice_phi_cartesian_x
AMRGridData_Slice_phi_cartesian_y
AMRGridData_Slice_phi_cartesian_z

Slices along theta (i.e., a conical section) of the three coordinate values:

AMRGridData_Slice_theta_cartesian_x
AMRGridData_Slice_theta_cartesian_y
AMRGridData_Slice_theta_cartesian_z

Slices along r (i.e., a 'surface') of the three coordinate values:

AMRGridData_Slice_r_cartesian_x
AMRGridData_Slice_r_cartesian_y
AMRGridData_Slice_r_cartesian_z

I'd really appreciate thoughts from @neutrinoceros and @jzuhone about these.

@matthewturk
Copy link
Member Author

(and @chrishavlin if he has the time!)

@neutrinoceros
Copy link
Member

This looks great, and is absolutely useful, thank you so much !
I note that plot bounds seem to be off in phi-normal and theta-normal slice plots, which has probably nothing to do with this PR but unless you set them manually to be that way, that would be a bug. Can you include the script you wrote to generate these ?

Comment on lines +84 to +86
self.setup_cartesian_fields(registry)

def setup_cartesian_fields(self, registry):
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem to be an existing method name in other CoordinateHandler classes and I can't see what we get from splitting this into a separate method, is there any benefit I'm not seeing ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm okay with it being in the same function. I was using this for debugging, and wasn't sure if we wanted to make it a property of the superclass.


registry.add_field(
("index", "cartesian_x"),
sampling_type="local",
Copy link
Member

Choose a reason for hiding this comment

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

why "local", and not "cell" ?

@matthewturk
Copy link
Member Author

@neutrinoceros yup, script is at top.

@neutrinoceros
Copy link
Member

sorry I missed it ! definitely a bug, I will look into this !

@chrishavlin
Copy link
Contributor

This is very useful! I was kind of struggling to understand what the plots should show exactly... so I started playing with using these fields to interpolate values to a regular cartesian grid: link to notebook. Seems to work pretty well!

A question: How would you feel about adding the same fields for geogrpahic datasets? the GeographicCoordinateHandler.setup_fields already adds theta, phi and r fields. Not exactly elegant, but just copy/pasting the fields you've added here over in the geopraphic handler should work. I don't really see a more elegant solution without refactoring the geographic handler significatnly (to e.g., inherit from the spherical handler).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants