-
Notifications
You must be signed in to change notification settings - Fork 119
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
Fms2 io implementation #74
Fms2 io implementation #74
Conversation
…cing the necessary functions with fms2_io_mod functions
…name when needed and removing unneccessary code in fv_io_mod
Lucas - I can run some tests and compare the timing. |
The performance degradation you are talking about was a fundamental issue
initially that has since been rectified. In fact, anything you are running
today with a recent release of FMS (2020.03 and newer) has the diagnostics
being output via the fms2_io interfaces. If you aren't seeing runtime
impacts at the end of a simulation when all diagnostic data is being pushed
to storage, it is unlikely to occur with the restarts. Regardless, a few
timing tests will be run to prove it.
|
@laurenchilutti - This PR was opened prior to a template being in place. There is a lot of good work here, but I want to make sure we've tested to the largest extent that we can. Could you list the tests you've run with the fms2_io to give an idea of the code coverage? In particular, it would be good to list the IC type (GFS, HRRR, ECMWF, SHiELD), model type (nested, regional, etc.) and whether restarts (intermediate writing and functional test) were a part of the testing as well. Also, are there changes coming/needed within the SHiELD_driver project? |
9063a74
to
fb96053
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.
Can you remove any references to the use association of write_version_number from fms_io_mod. I think the only place it appears is in tools/fv_diagnostics.F90 where it is also use associated via fms_mod.
@bensonr My apologies for not directly responding to the comment from 6 days ago. I am putting a list of all tests run and will update the description of this PR to include that. SHiELD_drivers will also require updates to fms2io that has not been finished yet. For now, the branch: user/lec/fms2io from my fork is needed to test fms2io with SHiELD. I will go ahead and clean up the use of write_version_mod. |
Hi, Lauren. Thanks for working on this.
Lucas
…On Wed, Apr 28, 2021 at 12:19 PM laurenchilutti ***@***.***> wrote:
@bensonr <https://github.com/bensonr> My apologies for not directly
responding to the comment from 6 days ago. I am putting a list of all tests
run and will update the description of this PR to include that.
SHiELD_drivers will also require updates to fms2io that has not been
finished yet. For now, the branch: user/lec/fms2io
<https://github.com/laurenchilutti/SHiELD_driver/tree/user/lec%2Ffms2io>
from my fork is needed to test fms2io with SHiELD.
I will go ahead and clean up the use of write_version_mod.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#74 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMUQRVFI4GDZVDGM6KLQ2KLTLAYSHANCNFSM4ZAY7JPQ>
.
|
@bensonr I do not see any use statements from fms_io_mod anywhere in this new code. When you look at tools/fv_diagnostics.F90 it used to use write_version_number from fms_io_mod, but I removed and replaced that with fms_mod. I may be misunderstanding your request. |
@laurenchilutti thanks for confirming. I was looking at the current code and didn't realize you'd fixed it as part of this PR. |
@laurenchilutti - do you feel ready for a formal review? |
@bensonr Yes, I am ready for a formal review. I updated the description to include testing completed. Note that I have not been able to test a nested run warm-starting from restarts - I am working with Joseph on this. |
@bensonr <https://github.com/bensonr> Yes, I am ready for a formal
review. I updated the description to include testing completed. Note that I
have not been able to test a nested run warm-starting from restarts - I am
working with Joseph on this.
We can add that as a note to the CHANGELOG.md for the updates.
|
…, addressding Uriel's comments from PR#74
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.
Thanks @laurenchilutti. It all looks good to me.
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.
This looks good. I am really glad we can get rid of the INTERNAL_FILE_NML. One question: has nggps_ic been tested with these changes?
@lharris4 - there is a "How this has been tested" section in the description of the PR (thanks to the PR template we appropriated from the NOAA-GFDL/FMS project). Please let us know if there is untested functionality that needs to be addressed. |
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.
I believe you still have an instance of INTERNAL_FILE_NML in fv_diag_column.F90
…ter_axis in fv_ada_nudge. Reintroducing flush function and updating namelist read in fv_diag_column
Description
This pull request implements fms2io into the GFDL_atmos_cubed_sphere code. In a coming release of FMS, the modules
mpp_io_mod
andfms_io_mod
are being deprecated and replaced by the newfms2_io_mod
. These changes go hand in hand with the changes to the physics in PR6.Fixes # (issue)
How Has This Been Tested?
These changes have been tested with SHiELD and do not change answers. Using FMS 2021.02-beta2 tag and SHiELD_driver laurenchilutti:user/lec/fms2io
Specific tests run are:
Checklist:
Please check all whether they apply or not