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 harmonics limiter for entr/detr, set GABLS config #2125

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Conversation

trontrytel
Copy link
Member

closing #2124

@trontrytel trontrytel requested a review from szy21 September 20, 2023 04:32
@trontrytel trontrytel self-assigned this Sep 20, 2023
@trontrytel trontrytel linked an issue Sep 20, 2023 that may be closed by this pull request
Copy link
Member

@szy21 szy21 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just want to double check that this filter is approved by @tapios :) Should we start to keep track of the filters we have somewhere?

Btw when I tried a quick hack for setting e_int to be the same it didn't work, but maybe I did something wrong.

Copy link
Contributor

@tapios tapios left a comment

Choose a reason for hiding this comment

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

I do not particularly like this zeroing out approach, but I'm ok with having it if there is no more principled alternative.

It should suffice to have entrainment go to zero as a -> 0. Did you try an approach that does that? In some experiments, I found that limiting entrainment by multiplying it with 2*harmonic_mean([max(a, 0); max(1-a, 0)])) (while increasing entrainment overall) leads to bounded solutions for a. Could you try this here?

src/prognostic_equations/filter_sgs_u3_tendency.jl Outdated Show resolved Hide resolved
@trontrytel
Copy link
Member Author

I do not particularly like this zeroing out approach, but I'm ok with having it if there is no more principled alternative.

It should suffice to have entrainment go to zero as a -> 0. Did you try an approach that does that? In some experiments, I found that limiting entrainment by multiplying it with 2*harmonic_mean([max(a, 0); max(1-a, 0)])) (while increasing entrainment overall) leads to bounded solutions for a. Could you try this here?

I will try it. But I think that here the issue is not that the area does not go to zero. But rather that the area is close to zero, but the updraft still has different properties than the grid mean

@tapios
Copy link
Contributor

tapios commented Sep 20, 2023

I do not particularly like this zeroing out approach, but I'm ok with having it if there is no more principled alternative.
It should suffice to have entrainment go to zero as a -> 0. Did you try an approach that does that? In some experiments, I found that limiting entrainment by multiplying it with 2*harmonic_mean([max(a, 0); max(1-a, 0)])) (while increasing entrainment overall) leads to bounded solutions for a. Could you try this here?

I will try it. But I think that here the issue is not that the area does not go to zero. But rather that the area is close to zero, but the updraft still has different properties than the grid mean

Is the issue the updraft buoyancy?

@trontrytel
Copy link
Member Author

Is the issue the updraft buoyancy?

The way I understand it is that if a<<1 we are setting the updraft e_tot to be the same as the grid mean. But if the updraft vertical velocity is different that will make internal energy different and will cause things to diverge. I'm not sure what is causing the initial divergence.

Here are the timeseries and final profiles from GABLS without the filter I'm trying to add.

GABLS_contours
GABLS_final_profiles

@szy21
Copy link
Member

szy21 commented Sep 27, 2023

It's probably not related, I just would like to comment here so I won't forget: I think it would be better to change this line to ᶠinterp(ᶜentrʲs.:($$j)[colidx]) * ᶠu₃⁰[colidx], so that we can make sure the entrainment tendency of updraft u_3 is zero when it is the same as grid-mean u_3.

@trontrytel trontrytel changed the title add the filter for GABLS Add harmonics limiter for entr/detr, set GABLS config Sep 27, 2023
@trontrytel
Copy link
Member Author

In the end I'm:

  • reducing the dt to 5s,
  • adding another entr/detr option: a constant coefficient but limited by the harmonics mean of (a, 1-a). This is so that we can easily test it for other cases.

This doesn't solve the problem. Our GABLS simulation continues to diverge. But maybe we can merge this setup in, while we try to understand how to make the scheme more stable.

Do we want to keep the harmonics limiter as default?

With harmonics:

contours_w_harmonics

Without harmonics:

contours_wo_harmonics

@szy21
Copy link
Member

szy21 commented Sep 28, 2023

bors r+

bors bot added a commit that referenced this pull request Sep 28, 2023
2125: Add harmonics limiter for entr/detr, set GABLS config r=szy21 a=trontrytel

closing #2124 

Co-authored-by: Anna Jaruga <ajaruga@caltech.edu>
@bors
Copy link
Contributor

bors bot commented Sep 28, 2023

Build failed:

@trontrytel
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Sep 28, 2023
2125: Add harmonics limiter for entr/detr, set GABLS config r=trontrytel a=trontrytel

closing #2124 

Co-authored-by: Anna Jaruga <ajaruga@caltech.edu>
@bors
Copy link
Contributor

bors bot commented Sep 28, 2023

Build failed:

@trontrytel
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Sep 28, 2023
2125: Add harmonics limiter for entr/detr, set GABLS config r=trontrytel a=trontrytel

closing #2124 

Co-authored-by: Anna Jaruga <ajaruga@caltech.edu>
@bors
Copy link
Contributor

bors bot commented Sep 28, 2023

Build failed:

@trontrytel
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Sep 28, 2023
2125: Add harmonics limiter for entr/detr, set GABLS config r=trontrytel a=trontrytel

closing #2124 

Co-authored-by: Anna Jaruga <ajaruga@caltech.edu>
@bors
Copy link
Contributor

bors bot commented Sep 28, 2023

Build failed:

@szy21
Copy link
Member

szy21 commented Sep 28, 2023

bors r+

bors bot added a commit that referenced this pull request Sep 28, 2023
2125: Add harmonics limiter for entr/detr, set GABLS config r=szy21 a=trontrytel

closing #2124 

Co-authored-by: Anna Jaruga <ajaruga@caltech.edu>
@bors
Copy link
Contributor

bors bot commented Sep 28, 2023

Build failed:

@trontrytel
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Sep 28, 2023
2125: Add harmonics limiter for entr/detr, set GABLS config r=trontrytel a=trontrytel

closing #2124 

Co-authored-by: Anna Jaruga <ajaruga@caltech.edu>
@bors
Copy link
Contributor

bors bot commented Sep 28, 2023

Build failed:

@trontrytel
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Sep 28, 2023
2125: Add harmonics limiter for entr/detr, set GABLS config r=trontrytel a=trontrytel

closing #2124 

Co-authored-by: Anna Jaruga <ajaruga@caltech.edu>
@bors
Copy link
Contributor

bors bot commented Sep 28, 2023

Build failed:

@trontrytel
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Sep 28, 2023
2125: Add harmonics limiter for entr/detr, set GABLS config r=trontrytel a=trontrytel

closing #2124 

Co-authored-by: Anna Jaruga <ajaruga@caltech.edu>
@bors
Copy link
Contributor

bors bot commented Sep 28, 2023

Build failed:

@trontrytel
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 29, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 8713fac into main Sep 29, 2023
6 checks passed
@bors bors bot deleted the aj/edmfx_gabls branch September 29, 2023 18:01
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.

Make GABLS work
3 participants