Add new slope-limited transport operator #3485
Conversation
config/model_configs/sphere_baroclinic_wave_rhoe_equilmoist.yml
Outdated
Show resolved
Hide resolved
charleskawczynski
force-pushed
the
as/vanleerlimiter
branch
from
1c8a225 to
614d8a7
Compare
charleskawczynski
force-pushed
the
as/vanleerlimiter
branch
from
819f565 to
8a1814b
Compare
|
@dennisYatunin, could you please look at this when you have a chance? |
|
Also, since this is supposed to be set as the new default, should we put a new requirement that ClimaAtmos requires the latest ClimaCore? Conditionally requiring this seems a bit confusing, since the default would also need to be different, and that's set in the toml file. Thoughts, @Sbozzolo, @dennisYatunin? |
|
One simple way to preserve compatibility with prior versions could be to
change the default in the yaml file, and if we detect that the climacore
version is below 0.14.21 in type_getter we emit a warning saying that the
limiter is not available and disable it. This would prompt users to upgrade
their climacore version, while also allowing people to run with old
climacore versions if they need to.
…On Sun, Dec 22, 2024, 8:20 PM Charles Kawczynski ***@***.***> wrote:
Also, since this is supposed to be set as the new default, should we put a
new requirement that ClimaAtmos requires the latest ClimaCore?
Conditionally requiring this seems a bit confusing, since the default would
also need to be different, and that's set in the toml file. Thoughts,
@Sbozzolo <https://github.com/Sbozzolo>, @dennisYatunin
<https://github.com/dennisYatunin>?
—
Reply to this email directly, view it on GitHub
<#3485 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACF6E7MYEOWGBOPOF4QOWHD2G6FRPAVCNFSM6AAAAABTZO6DHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJYHA3TQNBXG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
charleskawczynski
force-pushed
the
as/vanleerlimiter
branch
from
8a1814b to
28253f9
Compare
|
I can take a look tonight or tomorrow morning if Dennis hasn’t already. |
There was a problem hiding this comment.
Code changes look good to me! If @szy21 thinks the CI results are correct then this should be fine to merge in. I'm okay with bumping the required ClimaCore version since this has to be the new default, but Gabrielle's suggestion also works.
Have the longruns been checked against this branch yet? If not, we should probably do that before committing to the new default.
|
For some reason, the summary plots are not there for the jobs that break reproducibility tests (and it seems those tests break in the main branch too). I looked at some other tests and they look fine. Akshay tested this branch some time ago with the dycore longruns, so I think it would be ok. |
|
@charleskawczynski I'll bump the ref counter to get some plots and see if the reproducibility test error disappears. |
|
Alright, the ci results look good to me. The ClimaCoupler downstream tests break because of the ClimaCore version I think. |
|
Rebasing... |
charleskawczynski
force-pushed
the
as/vanleerlimiter
branch
3 times, most recently
from
06fb52c to
bb01d35
Compare
modified: config/model_configs/sphere_baroclinic_wave_rhoe_equilmoist.yml modified: examples/Manifest.toml modified: src/prognostic_equations/implicit/implicit_tendency.jl modified: src/utils/abbreviations.jl modified: NEWS.md Fixes Make default Fixes bump ref counter Compat fixes
charleskawczynski
force-pushed
the
as/vanleerlimiter
branch
from
bb01d35 to
da85255
Compare
Adds new operator that uses constrained slope limiter methods, following Lin et al. (1994) Equations (1b,1c, 2, 5).
Updates moist baroclinic wave problem to use this limiter configuration.