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

Add functionality for spherical system #5629

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

Conversation

alarshi
Copy link
Contributor

@alarshi alarshi commented Apr 19, 2024

This PR addresses one of the bullet points in issue #5626. In particular, it allows the user to use spherical coordinate system while using initial_topography function model.

Before your first pull request:

For all pull requests:

Assert (coordinate_system == Utilities::Coordinates::CoordinateSystem::cartesian,
ExcNotImplemented());
ExcMessage("While using a box geometry, use cartesian coordinate system."));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to say

Suggested change
ExcMessage("While using a box geometry, use cartesian coordinate system."));
ExcMessage("While using a box geometry, you can only use the cartesian coordinate system."));

?

Comment on lines +81 to +82
Assert (coordinate_system == Utilities::Coordinates::CoordinateSystem::spherical,
ExcMessage("Make sure that you are using a spherical coordinate system While using a spherical geometry."));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Assert (coordinate_system == Utilities::Coordinates::CoordinateSystem::spherical,
ExcMessage("Make sure that you are using a spherical coordinate system While using a spherical geometry."));
Assert (coordinate_system == Utilities::Coordinates::CoordinateSystem::spherical,
ExcMessage("Make sure that you are using a spherical coordinate system while using a spherical geometry."));

Comment on lines +81 to +82
Assert (coordinate_system == Utilities::Coordinates::CoordinateSystem::spherical,
ExcMessage("Make sure that you are using a spherical coordinate system While using a spherical geometry."));
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the assertion to the top of the code block. Everything in this block is wrong if the assertion is not satisfied, we might as well abort there.

@gassmoeller
Copy link
Member

@alarshi: This is actually not quite the implementation I had in mind in #5626. I was thinking we should allow users to choose which coordinate system to use (as in boundary_temperature/function.cc), not force them to use the coordinate system corresponding to their chosen geometry model. It is possible that that is not the right approach for initial_topography, but can you think about it and let me know what you think is most useful?

@alarshi
Copy link
Contributor Author

alarshi commented May 29, 2024

@gassmoeller : I see. I can think of cases where a user might want to use the initial topography from another model run (fastscape?) as an input to ASPECT models. I agree that having a choice between cartesian/spherical would be useful.
I will add that, thanks for the suggestion!

@alarshi
Copy link
Contributor Author

alarshi commented May 30, 2024

@gassmoeller : Hmm, I see why the function in initial_topography was implemented based on the geometry model we are using. Unlike, the boundary_temperature and heating_model implementations in which we compute the boundary values at a point, initial_topography computes boundary values for a surface_point. This implies that in order to use the Function Parser, we need convert the surface_point to a point and this will depend on the geometry model.
Let me know if you have other thoughts or I am misunderstanding what you said.

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.

3 participants