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

fix for CatchCN CLM4.5 fail on SLES15 GNU #898

Closed
wants to merge 4 commits into from

Conversation

biljanaorescanin
Copy link
Contributor

@biljanaorescanin biljanaorescanin commented Feb 7, 2024

During SLES15 testing we got CatchCN clm 4.5 model run fail due to floating-point exception.
Issue happens during specific calculation where exponent is too large.

Error is in this calculation:

fire_m_tmp = exp(-fire_m_fac(ivt(p))*(m/0.69_r8)**2)*(1.0_r8 - max(0._r8, &
min(1._r8,(forc_rh(g)-30._r8)/(80._r8-30._r8))))* &
min(1._r8,exp(SHR_CONST_PI*(forc_t(g)-SHR_CONST_TKFRZ)/10._r8))

Issue: #896

@gmao-jkolassa this fix works and results in round off difference for CatchCN. Maybe there is better way to do it I am not sure.

For GCM this is zero diff trivial since fix is in CLM4.5 module.

fyi: @gmao-rreichle @rdkoster

@biljanaorescanin biljanaorescanin added 0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) Not 0-diff for CatchCN labels Feb 7, 2024
@gmao-rreichle
Copy link
Contributor

@gmao-jkolassa : We'd like to get feedback the following:

  • Is there a specific reason why fire_m_fac(4,6) was set to the ridiculously large value (10.e15), which ends up in the exponent in the fire_m_tmp calculation?
  • Would you be ok with the new value (10.e5)?
  • A better solution is probably to set fire_m_fac(4,6) to no-data, then avoid the calculation of exp(-fire_m_fac*...) altogether and set fire_m_tmp to whatever you think is appropriate in this case [or prescribe the result of exp(-fire_m_fac*...) for the PFTs in question]

@gmao-rreichle gmao-rreichle added the bugfix This fixes a bug label Feb 7, 2024
@gmao-jkolassa
Copy link
Contributor

@biljanaorescanin Thank you for implementing this fix. I am happy with this solution for now.

@gmao-rreichle , to answer your questions:

Is there a specific reason why fire_m_fac(4,6) was set to the ridiculously large value (10.e15), which ends up in the exponent in the fire_m_tmp calculation?

The fire_m_fac parameter controls the sensitivity of combustibility to the soil moisture conditions. The large value of 10e15 was chosen to address excessive amounts of fires simulated for PFTs 4 and 6. At the time we tested a few different values for fire_m_fac and found that only by limiting the occurrence of fire in PFTs 4 and 6 to very dry conditions were we able to get a somewhat reasonable match to observations.

Would you be ok with the new value (10.e5)?

Because of the above issue and many other problems with the CLM4.5 fire code that we found, we decided to no longer conduct fire simulations with this code and instead moved forward the implementation of Catchment-CN5.1, with its (arguably) improved fire code. That is to say, no one is currently using Catchment-CN4.5 for fire simulations and so I think setting fire_m_fac to 10.e5 to avoid the issues that were encountered during testing is fine.

A better solution is probably to set fire_m_fac(4,6) to no-data, then avoid the calculation of exp(-fire_m_fac*...) altogether and set fire_m_tmp to whatever you think is appropriate in this case [or prescribe the result of exp(-fire_m_fac*...) for the PFTs in question]

I agree that there probably is a better way to address this issue. At the time we were discussing to also vary the parameters controlling the sensitivity of the combustibility to the atmospheric humidity and atmospheric temperature instead of just altering the soil moisture sensitivity alone (which led to having to set it to this very large value). But because of all the other issues mentioned above, we never did test this and instead decided to move on to a newer model. For the same reason and given that Catchment-CN5.1 is around the corner, I am not sure whether it makes sense to spend a lot of time on coming up with a better fix. But, I will let @rdkoster chime in on that point.

@rdkoster
Copy link
Contributor

rdkoster commented Feb 7, 2024 via email

@gmao-rreichle
Copy link
Contributor

Per @gmao-jkolassa's report of older results from the time when the original 10e15 numbers were implemented, this change would impact the science output of CNCLM45 and thus cannot be merged without proper science evaluation. Since NCAR advises that CLM45 should not be used anymore, the plan is to disable CNCLM45 in GEOS going forward by disallowing LSM_CHOICE=3 in the setup of the GCM and the LDAS. This will be done in separate PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) bugfix This fixes a bug Not 0-diff for CatchCN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants