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: Fix typo in vector operations and enable plot modifications for spherical geometry #4378

Merged
merged 4 commits into from
Mar 20, 2023

Conversation

jisuoqing
Copy link
Member

@jisuoqing jisuoqing commented Mar 20, 2023

  • Fixed typos in vector operations in spherical geometry
  • Enable Streamline and LineIntegralConvolution callbacks for spherical geometry
  • Callback tests added

Some further thoughts -- when using Quiver, Streamline and LineIntegralConvolution callbacks for curvilinear geometries, we assume users know what vector fields to plot (_cartesian_x, _conic_x, _cylindrical_radius etc.), while this is obscure and hidden in the source code. Maybe improvement could be made in future to generate a warning message when users try to use inappropriate vector fields in those callbacks.

Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Thanks ! could you provide a simple demo script so we can verify that Streamlines and LIC callbacks just work ?

@@ -529,7 +529,7 @@ def _cartesian_x(field, data):
* np.cos(data[(ftype, "phi")])
+ data[(ftype, f"{basename}_theta")]
* np.cos(data[(ftype, "theta")])
* np.cos([(ftype, "phi")])
* np.cos(data[(ftype, "phi")])
Copy link
Member

Choose a reason for hiding this comment

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

oof, thank you for catching these !

@jisuoqing
Copy link
Member Author

Sure! Let me add a test for this recently.

@neutrinoceros
Copy link
Member

Some further thoughts -- when using Quiver, Streamline and LineIntegralConvolution callbacks for curvilinear geometries, we assume users know what vector fields to plot (_cartesian_x, _conic_x, _cylindrical_radius etc.), while this is obscure and hidden in the source code.
Maybe improvement could be made in future to generate a warning message when users try to use inappropriate vector fields in those callbacks.

+1/2 on this. Available fields could certainly be better documented, but I think Quiver, Streamline and LineIntegralConvolution callbacks should in principle accept any pair of scalar fields (i.e., not flag any usage as "inappropriate").

@jisuoqing
Copy link
Member Author

+1/2 on this. Available fields could certainly be better documented, but I think Quiver, Streamline and LineIntegralConvolution callbacks should in principle accept any pair of scalar fields (i.e., not flag any usage as "inappropriate").

I agree that we won't throw an error, while it's unphysical, say, to plot velocity_r and velocity_theta on a plot along the phi-axis in spherical geometry (and the physical vector fields should be velocity_cylindrical_radius and velocity_cylindrical_z). Should a warning message appear to give a hint for the users?

@neutrinoceros
Copy link
Member

neutrinoceros commented Mar 20, 2023

Should a warning message appear to give a hint for the users?

IMHO it shouldn't. While velocity vectors and magnetic fields are by far the most common use cases for these callbacks, I don't think that we should invalidate creative usage in any way (no errors, no warnings).

@jisuoqing
Copy link
Member Author

jisuoqing commented Mar 20, 2023

Well I just browsed the public dataset list on yt website, and cannot find a 3D spherical one with a non-zero coherent velocity/magnetic field, so I display a plot from my data as follows, and update some testing script as well.

halo out1 00000_Slice_phi_magnetic_field_strength

@neutrinoceros
Copy link
Member

pre-commit.ci autofix

@neutrinoceros neutrinoceros enabled auto-merge March 20, 2023 16:48
@neutrinoceros neutrinoceros merged commit 11cb4be into yt-project:main Mar 20, 2023
@jisuoqing jisuoqing deleted the annotation branch March 21, 2023 00:43
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