-
Notifications
You must be signed in to change notification settings - Fork 9
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 implicit tendency for canopy temperature #675
base: main
Are you sure you want to change the base?
Conversation
150b23a
to
eadfba1
Compare
222a4c7
to
c7aaf40
Compare
c06b355
to
aa327e9
Compare
8323271
to
f754df6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good overall, but I have some concerns about the reliability of the tests. The current test results indicate that there are significant errors in the Jacobian approximation, but these errors might appear bigger than they actually are if there is a problem with the reference solutions. This issue can be dealt with in future PRs, though; the new infrastructure added here is fine to merge in.
finitediff_LHF = (p_2.canopy.energy.lhf .- p.canopy.energy.lhf) ./ ΔT | ||
estimated_LHF = p.canopy.energy.∂LHF∂qc .* p.canopy.energy.∂qc∂Tc | ||
@test parent(abs.(finitediff_LHF .- estimated_LHF) ./ finitediff_LHF)[1] < | ||
0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These Jacobian errors seem rather large to me. Although they're most likely caused by issues with your approximation, they could also be coming from the reference finite difference solution if your system is sufficiently nonlinear. I don't have a good intuition for whether this is the case here, but you can check by using autodiff for the reference instead of finite differences. I can help you set that up using ForwardDiff.jl
if you think it's worth looking into.
available_explicit_vars = | ||
MatrixFields.unrolled_filter(is_in_Y, explicit_vars) | ||
|
||
get_jac_type( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a little risky to determine the jacobian block type based on the space rather than the variable name. Unless you are certain that your 3D variable derivatives will always be tridiagonal (which is not the case in ClimaAtmos), I would recommend dispatching on variable names here instead.
|
||
# Set up timestepper | ||
timestepper = CTS.ARS343(); | ||
timestepper = CTS.ARS111(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you encounter some sort of issue with the ARS343
timestepper here (and in other experiments), or did you just switch to IMEX Euler for convenience?
ed80730
to
c638d87
Compare
b4e83f3
to
ddbc379
Compare
Some insights we gained during explicitly-stepped experiments in #771 - In testing the canopy temperature implicit solver implemented in #675, we saw unexpected convergence behavior. To investigate if the cause of this was due to inacurracy of the reference solution or flaws in the simulation setup, we take a look here at the convergence behavior of the same setup using an explicit solver. Key point 1:
|
Which convergence checker/max iters combo are we using? this is with ARS111? Just curious because, as this looks quite good already, I am wondering if we can improve the error |
This is using ARS111 with |
7dbc323
to
204a683
Compare
Here are plots showing the difference in T at the end of a 100 day simulation between runs with dt=10s and dt=1800s. The plot on the left uses |
thanks for checking this! I think this is consistent with there being certain times where the Newton iterations do not converge (and not because we dont carry out enough iterations, but because J is so far off) |
c19838d
to
6e300b0
Compare
5ec872e
to
fb73d27
Compare
Purpose
Add implicit tendency for canopy temperature
Address #696
There are two things I dont understand about this change:
These two questions will be investigated in a later PR - see #782
Results
With an explicit stepper, RK4, a test script at the Ozark site with the canopy alone permits dt = 60 [ac_canopy = 1e3]. Time to solution = 114 seconds - see kd/canopy_explicit_tests branch. Using dt = 180 fails.
With an implicit stepper ARS111, updated jacobian every newton iteration, max_iters = 6, the same simulation can be run with dt=3600. Time to solution = 16 seconds. Using only a single iteration failed.
Accuracy as a function of timestep (ref solution = dt = 6, implicit solver)
Float 32:
Float64:
In the global long run, on this branch: kd/canopy_explicit_global_nans I just changed the value of ac_canopy but left everything else the same as #main - i.e. using an explicit solver. The resulting map is almost entirely NaN [ look at time 86400]
On the implicit branch (this PR), we have no NaNs after one day. [ look at time 86400]
To-do
Content