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

Redesign saturation table initialization #303

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

laurenchilutti
Copy link
Contributor

Description

Bringing updates made by Linjiong Zhou to the main branch. Will merge to the dev/gfdl_am5 branch.

Call qs_init in the AM model when the GFDL MP isn't used.
Remove the redundant qs_init in the GFDL MP.

Fixes # (issue)

How Has This Been Tested?

Tested by FV3 Team developers.

Checklist:

Please check all whether they apply or not

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Redesign saturation table initialization

See merge request fv3team/atmos_cubed_sphere!140
@bensonr
Copy link
Contributor

bensonr commented Oct 27, 2023

@laurenchilutti - has this been previously fixed in the SHiELD driver?

@linjiongzhou
Copy link
Contributor

@bensonr, this fix is designed for the AM model. We have gfdl_mp_init in the SHiELD driver, so we don't need qs_init.

@laurenchilutti
Copy link
Contributor Author

@bensonr To summarize the detail Linjiong gave me:

In AM5, when the relative humidity diagnostic was turned on, the model blew out with a segmentation fault error. This error pointed to the GFDL MP saturation water vapor table.

It turns out that the saturation water vapor table wasn't initialized in AM5, and our original code to fix this with "if (.not. tables_are_initialized) call qs_init" was useless within AM5.

Therefore, Linjiong proposed calling qs_init in AM5 during the initialization, similar to the procedure for calling gfdl_mp_init during initialization in SHiELD. Baoqiang completed testing on this approach and confirmed that it resolved the issue.

@bensonr
Copy link
Contributor

bensonr commented Oct 27, 2023

@laurenchilutti @linjiongzhou - wouldn't it have been easier to make sure it was initialized from within fv_diagnostics? qs_init is already included via module use within fv_diagnostics and it's a matter of expanding what diag ids require it. I'm also assuming the tables are initialized once and have protections about them. It's possible we could remove the #if/#endif pair and always make sure the tables are initialized.

@linjiongzhou
Copy link
Contributor

@bensonr, the saturation table is not only used in the diagnostic but also in other places that involve saturation calculation. For example, the 2dx filter and negative tracer removal use the saturation table. That is why I intended to put it in the initialization process.

@bensonr
Copy link
Contributor

bensonr commented Oct 27, 2023

@linjiongzhou - fv_diag_init comes 12 lines after the existing qs_init in atmosphere_init. If the qs_init is not needed by the fv_restart process, then it'll be initialized by the diagnostics. If you want to always initialize qs_init in atmosphere_init, then let's consider removing the one fv_diagnostics_init.

@laurenchilutti
Copy link
Contributor Author

@bensonr Let me know if this is ready to merge, or if you would like @linjiongzhou to make changes

@linjiongzhou
Copy link
Contributor

@bensonr, thanks for your suggestion. I can take the "call qs_init" off the fv_diag_init. It is safer to put it in the atmosphere_init. Thanks!

@bensonr
Copy link
Contributor

bensonr commented Oct 27, 2023

@linjiongzhou - wfm. I'll approve this one now and we'll fix the diagnostics in another PR

@laurenchilutti laurenchilutti merged commit efcb02f into NOAA-GFDL:main Oct 27, 2023
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants