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

set bubble to false #3516

Merged
merged 1 commit into from
Jan 9, 2025
Merged

set bubble to false #3516

merged 1 commit into from
Jan 9, 2025

Conversation

szy21
Copy link
Member

@szy21 szy21 commented Jan 7, 2025

Purpose

Our current implementation of the bubble correction is not internally consistent, as it modifies the volume element J without modifying the metric tensor g, so that we no longer satisfy the discrete identity J = sqrt(det(g)). Our current implementation also does not account for volume distortions due to topography, as it is only applied to the horizontal component of the volume element. In order to fix this (and be more similar to HOMME's implementation), we would need to apply the bubble correction to the full 3D metric Jacobian, and then compute J and g from the corrected Jacobian.

However, as Paul Ullrich pointed out, modifying g on interior nodal points without modifying it on boundary nodal points will have the consequence of changing the discrete curvature. I think this is basically the point of the bubble correction (trading accuracy in curvature for accuracy in volume integrals), but Tapio is not a fan of this tradeoff. Our current plan is to eliminate the bubble correction from ClimaCore altogether, and instead handle volume integral discrepancies during remapping, which will allow us to avoid changes in curvature terms.

We might end up revisiting the bubble correction later if this strategy does not work well, but we will still need to implement it in a completely different way to fix the current issues. So, there is no point in keeping the current implementation.

To-do

Content


  • I have read and checked the items on the review checklist.

@charleskawczynski
Copy link
Member

Fine by me. Why did we decide that?

@szy21
Copy link
Member Author

szy21 commented Jan 7, 2025

Fine by me. Why did we decide that?

@dennisYatunin ?

@dennisYatunin
Copy link
Member

@charleskawczynski I just updated the PR description with some details about why we are removing the current version of the bubble correction.

@szy21 szy21 enabled auto-merge January 8, 2025 20:59
@szy21 szy21 added this pull request to the merge queue Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@szy21 szy21 added this pull request to the merge queue Jan 9, 2025
Merged via the queue into main with commit 3c6181a Jan 9, 2025
15 of 16 checks passed
@szy21 szy21 deleted the zs/no_bubble branch January 9, 2025 04:40
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