-
Notifications
You must be signed in to change notification settings - Fork 33
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
Few small fixes to Thompson MP #69
Conversation
… on multiple lines
… was increased prev); make fewer explicit rain drop breakup from collisions with graupel when T above 0C; fix so snow/graupel only sublimate when not melting
@gthompsnWRF Is there any reason not to combine this with #68 ? |
I had absolutely considered doing that. Should I just merge in the rain evap into this branch and close out 68 in favor of this one containing all 4 items? |
Yes, please. I'll update fv3atm and ufs-weather-model PRs from #68 to point to this instead once you do. |
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.
Looks good.
Done. I closed 68 after merging that change into this branch. |
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 looks like the code changes do exactly as described. Thanks for the detailed explanation.
Hang on just a little bit longer with this PR. Eric and I continue to await simulation results to reduce the chance we may wish to change the tuning knob of 5x vs. 1.5x in bullet point 3 as well as one other constant change we may introduce based on the case study. We should have answers today/tomorrow I think. |
OK, that's fine. You can turn this into a "draft PR" until you're finished. That is a good way to let us know that you're working on it still so that we don't kick off tests, etc. in the future. |
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.
Looks good
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.
Looks good to me. Thanks!
@gthompsnWRF @ericaligo-NOAA Please let us know here in the PR comments when this is ready to go through regression testing and enter the merge queue. |
@grantfirl Unfortunately we had a simulation crash in GFS so please hold off on making this merge. We are seeking the cause to fix as rapidly as possible. We narrowed down the crash is due to bullet number 3 in the description. So at least we know where to focus our attention. |
@gthompsnWRF what kind of crash in GFS (segmentation fault or out-of-memory)? |
Crash was caused by instability.
*940: FATAL from PE 940: NaN in input field of mpp_reproducing_sum(_2d),
this indicates numerical instability*
…On Wed, May 24, 2023 at 3:38 PM Qingfu Liu ***@***.***> wrote:
@gthompsnWRF <https://github.com/gthompsnWRF> what kind of crash in GFS
(segmentation fault or out-of-memory)?
—
Reply to this email directly, view it on GitHub
<#69 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFOCLERYDFL76CEDFYMMTTXHZPUDANCNFSM6AAAAAAXWA2S24>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Ruiyu Sun, PhD
Lynker at NOAA/NWS/NCEP/EMC
5830 University Research Ct., Rm. 2097
College Park, MD 20740
***@***.*** ***@***.***>
301-683-3787
|
@RuiyuSun Thank you very much for the information |
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.
Changes to prv_rev
will be reverted back to the original code after discussion with @gthompsnWRF
Rollback changes to rain evaporation
@AndersJensen-NOAA @gthompsnWRF @RuiyuSun Does Anders' commit fix the crash, or was this a separate issue? |
I think it is separate issue. But I can test it's impacts. |
@RuiyuSun This rollback is not expected to fix the crash. Do you have any updates from testing, and can you provide any additional information or output so that I can help diagnose the crash? |
Thank you @AndersJensen-NOAA. The error message is The log file is gfsfcst.log at /scratch1/NCEPDEV/stmp2/Ruiyu.Sun/ROTDIRS/cp8tb3n4revso/logs/2020120300. |
After adding the prr_sml(k) = 0.0 and prr_gml(k) = 0.0 in the two elseif blocks (Eric suggested, Greg confirmed) the global workflow experiment completed successfully. This file compares the results with and without the changes in the PR69. |
@gthompsnWRF @RuiyuSun I don't see the changes that you mention committed. If you want, I can do this and submit a new PR (giving you credit, of course) that I will be responsible for updating as the merge queue moves through other PRs. |
Ruiyu is taking AL until July 17. Either @ericaligo-NOAA or @gthompsnWRF can go ahead to update the code or wait for Ruiyu coming back to work on this PR. Xiaqiong Zhou at EMC has also tested these changes for an idealized case and confirmed the impact of the changes is small. We now feel confident that these changes can be merged into the model code base. |
I will see if I can make the commit into the repo. Stand by please. |
…on happens instead of melting
I believe this is now resolved and ready for next steps. I edited the top description paragraph to remove the one item that turned out to be fine (no bug) regarding rain evaporation tendency terms having proper units. A shout out to @ericaligo-NOAA for his strong help in diagnosing the problem with melting/sublimating snow and graupel. |
Looks good. |
There are a lot of approvals here, but none approving the more recent changes. Please get reviews for the latest code changes. Thanks. |
The code looks fine to me. |
On weather model RT tests, it shows |
@grantfirl testing is finished on UFS-WM PR #1734. Please feel free to merge this PR. |
This PR includes 3 small fixes to Thompson microphysics:
The minimum size of snow was increased (some months ago) but it was noted that the min size of snow is used to determine the upper-most size bin of cloud ice. It was set to 5 times the min size of snow, which is exceedingly large for the cloud ice category. Therefore, the constant was reduced to 2 - making cloud ice largest size bin of 600 microns which is plenty big enough, giving more resolution to all the ice size bins.
In testing RRFS with @ericaligo-NOAA and @RuiyuSun for a specific case study, it was noted that rain was evaporating with a large rate producing a temperature tendency of 27C/hour. We traced the issue to an extremely large number of rain drops that appears to be produced by colliding rain and graupel - which has an explicit drop breakup (splash if you will) from the collisions making 5 drops per collision. This was forcing the rain (formed by melting graupel) to tends towards 500 microns (0.5 mm) which is too small. So the constant used for collisions was reduced to 1.5 in an effort to keep rain larger (thus reducing evaporation).
There was an oversight in the code for melting snow and graupel. When RH is below saturation, the wetbulb temperature could be below 0C in which case ice does not melt. Instead, the ice (snow/graupel) will sublimate first until wetbulb temperature goes above melting. So the code fix now enforces that either melting occurs or sublimation, not both.