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

Remove a function that is never used. #5468

Merged
merged 2 commits into from
Nov 1, 2023
Merged

Conversation

bangerth
Copy link
Contributor

@bangerth bangerth commented Nov 1, 2023

Both of the chunk geometry variations have a function set_topography_model() that is, however, not used anywhere other than in one test where we set a zero topography -- which should not actually do anything useful.

As mentioned in #5467, I would like to get rid of explicitly storing manifolds because this leads to problems: What matters is the state of the manifold object when we create the triangulation because the triangulation copies the manifold. Storing a manifold explicitly invites problems when one later modifies the manifold and expects that that affects what the triangulation sees. It's better to get rid of the explicit objects, and removing these functions is one step towards that goa.

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 for cleaning up! I didnt know the triangulation keeps a copy of the manifold and uses that one. Does that mean it is not possible to modify the manifold later, i.e. when topography changes?

@bangerth
Copy link
Contributor Author

bangerth commented Nov 1, 2023

It didn't use to keep a copy, but a reference to the manifold given. But that's been years since that was changed.

And yes, modifying the manifold later on has no effect on the triangulation. That's exactly what I want to clean up, because keeping a manifold object around suggests that you can change it and it having an effect, when it reality it doesn't. I want to make that clear in the code by only creating these manifold objects as temporaries.

@gassmoeller
Copy link
Member

Ok, makes sense. Good to go then.

@gassmoeller gassmoeller merged commit a6c0edd into geodynamics:main Nov 1, 2023
5 checks passed
@bangerth bangerth deleted the remove branch November 1, 2023 18:41
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