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

Change order of spherical unit vectors from theta, phi to phi,theta #2820

Merged
merged 3 commits into from Feb 21, 2019

Conversation

anne-glerum
Copy link
Contributor

When setting up a chunk model with boundary velocities using the function plugin, I noticed that although the variables in the spherical coordinate system are interpreted as radius, longitude and latitude, the spherical unit vectors point in the direction of radius, latitude and then longitude. It seems more intuitive and in line with the general order of spherical coordinates in aspect to switch to radius, longitude, latitude. Did I understand this correctly and would you agree @bartniday ?

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Thanks, I absolutely agree we should get this right before a release. I would prefer to reorder the terms in the equations a bit more as suggested (no functional change), but then this should be good to go. Thanks for the cleanup 👍

- std::sin(phi)*u_theta
- std::cos(phi)*std::cos(theta)*u_phi; // X
+ std::cos(theta)*std::cos(phi)*u_theta
- std::sin(phi)*u_phi; // X
Copy link
Member

Choose a reason for hiding this comment

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

Could you make the reordering consistent by also here and below always using a*u_r + b*u_phi + c*u_theta, and order the individual terms as cos(phi)cos(theta) instead of cos(theta)cos(phi)?

@anne-glerum
Copy link
Contributor Author

@gassmoeller I've addressed your comment :)

@gassmoeller
Copy link
Member

Thanks 👍

@gassmoeller gassmoeller merged commit cc8d24f into geodynamics:master Feb 21, 2019
@anne-glerum anne-glerum deleted the spherical_unit_vectors branch February 22, 2019 13:50
@anne-glerum
Copy link
Contributor Author

@gassmoeller Just a question, existing prm files from users will be interpreted differently now, do we note that down somewhere?

@gassmoeller
Copy link
Member

Good point. Usually we note that in the changelog entries, and if possible also provide an update script in doc/update_scripts. However, this feature was added after the latest release, so we do not need to do either. But could you update @bartniday 's changelog entry with the new coordinate directions in a follow up PR? It is in doc/modules/changes/20180624_niday

@anne-glerum anne-glerum mentioned this pull request Mar 8, 2019
freddrichards pushed a commit to freddrichards/aspect that referenced this pull request May 20, 2019
…ectors

Change order of spherical unit vectors from theta, phi to phi,theta
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants