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: Always use the coordinate system normal for computing angular momentum components for particles #4872

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

jzuhone
Copy link
Contributor

@jzuhone jzuhone commented Apr 10, 2024

PR Summary

The current code for computing angular momentum components for particle fields based on their positions and velocities has an incorrect assumption. That is, if there is a normal field parameter (such as in the case of disk objects), that this is the normal vector that should be considered in the computation of the angular momentum. If there is no normal parameter, the normal vector is assumed to be [0,0,1]. This normal vector along with the center position is used to compute the components of position and velocity for the computation of the angular momentum vector.

The problem with using the normal vector from the field parameter (if it exists) is that it involves rotating the coordinate system to one other than the default cartesian coordinate system from the dataset, but in fact the latter is the coordinate system that we want the vector components in, so we do not want to rotate. The solution is to assume that normal is always [0,0,1] for Cartesian coordinate systems.

FWIW, the computation of the gas angular momentum in yt/fields/angular_momentum.py does not use the normal vector at all and thus this change brings the calculation of the angular momentum for both particles and gas into agreement with each other.

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@jzuhone jzuhone added the bug label Apr 10, 2024
@chrishavlin
Copy link
Contributor

@yt-fido test this please

@chummels
Copy link
Member

I'm assuming that this doesn't change the actual normal vector for the data object, which should remain what the geometry of the object defines. So this change just means that the angular momentum vector is correctly calculated for particles in an object in the same way it is for gas? Sounds good to me.

@jzuhone
Copy link
Contributor Author

jzuhone commented Apr 17, 2024

@chummels that's correct.

@chummels chummels merged commit d046de2 into yt-project:main Apr 17, 2024
13 checks passed
@neutrinoceros neutrinoceros added this to the 4.4.0 milestone Apr 17, 2024
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.

4 participants