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

Approximate soil energy Jacobian #678

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Conversation

kmdeck
Copy link
Member

@kmdeck kmdeck commented Jun 25, 2024

Purpose

Add in our first approximation for the energy equation Jacobian, which only includes d\rho e_int/dT.

This does allow us to take much bigger timesteps, more comparable to our target of 15 minutes, except in the cases where the soil is freezing or the canopy is present. If we make the thermal mass of the canopy larger, we can take bigger steps.
-> need to think about how we handle soil freezing in terms of implicit stepping
-> canopy temp probably does need an implicit solver.

To-do

Content

  • Alter imp/exp tendencies for soil energy hydrology
  • Alter update jacobian for soil energy hydrology
  • Increase timesteps in tutorials and some experiments. In cases with a canopy, this involved increasing ac_canopy as well, so those are unchanged
  • Add unit test/update unit tests
  • Adds a buildkite run showing performance as a function of timestep, similar to what we had in Richards equation. We simulation the exact same system, but in one case with T > T freeze, and in one case with T < Tfreeze. The timestep required is orders of magnitude different.
    No Phase change:
    energy_conservation_flux_full_soil_no_pc
    water_conservation_flux_full_soil_no_pc

With Phase change:
energy_conservation_flux_full_soil_pc
water_conservation_flux_full_soil_pc

Note the differences in x and y axes. The solution at dt = 10, 100 seconds for the phase change one has lots of oscillations and is not good enough, although the relative error is not terrible


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

@kmdeck kmdeck added the Run benchmarks Add this label to run benchmarks on clima label Jun 25, 2024
@kmdeck kmdeck self-assigned this Jun 25, 2024
@kmdeck kmdeck added the AMIP label Jun 25, 2024
@kmdeck kmdeck linked an issue Jun 25, 2024 that may be closed by this pull request
@kmdeck kmdeck requested a review from juliasloan25 June 26, 2024 19:55
Copy link
Member

@juliasloan25 juliasloan25 left a comment

Choose a reason for hiding this comment

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

This looks great!! Thank you Kat

src/standalone/Soil/energy_hydrology.jl Outdated Show resolved Hide resolved
test/standalone/Soil/soiltest.jl Show resolved Hide resolved
docs/tutorials/standalone/Soil/evaporation.jl Show resolved Hide resolved
docs/tutorials/standalone/Soil/freezing_front.jl Outdated Show resolved Hide resolved
experiments/benchmarks/land.jl Outdated Show resolved Hide resolved
experiments/standalone/Soil/full_soil_implicit.jl Outdated Show resolved Hide resolved
experiments/standalone/Soil/full_soil_implicit.jl Outdated Show resolved Hide resolved
experiments/standalone/Soil/full_soil_implicit.jl Outdated Show resolved Hide resolved
@kmdeck
Copy link
Member Author

kmdeck commented Jun 27, 2024

@juliasloan25 thank you for the review! I wont be able to address your comments until next week, but if you have time, let me know if you think these are the main things, or if you see any issues with how we defined J. I think it's all OK because it was just adding terms copying what we do for theta_l, but it would be helpful if you took a close look at that part too.

@kmdeck kmdeck force-pushed the kd/js/implicit_temperature branch from 778ef22 to 0ffc695 Compare July 1, 2024 17:44
@kmdeck kmdeck requested a review from dennisYatunin July 1, 2024 18:14
@kmdeck kmdeck force-pushed the kd/js/implicit_temperature branch from a88da3d to 87bcfbe Compare July 2, 2024 00:04
Copy link
Member

@dennisYatunin dennisYatunin left a comment

Choose a reason for hiding this comment

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

These changes all look good!

Comment on lines +258 to +259
top = Operators.SetValue(Geometry.WVector.(heat_top_flux_bc)),
bottom = Operators.SetValue(Geometry.WVector.(heat_bottom_flux_bc)),
Copy link
Member

@dennisYatunin dennisYatunin Jul 2, 2024

Choose a reason for hiding this comment

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

Just a note though that this will allocate two surface Fields on every implicit tendency update. It would generally be better to cache these Fields somewhere, or to redefine heat_top_flux_bc and heat_bottom_flux_bc to be Fields of WVectors instead of Fields of scalars.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, Dennis! good catch!

@kmdeck kmdeck merged commit cfa1030 into main Jul 2, 2024
11 checks passed
@kmdeck kmdeck deleted the kd/js/implicit_temperature branch July 2, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMIP Run benchmarks Add this label to run benchmarks on clima
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add implicit tendency for EnergyHydrology energy
3 participants