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.

Sign up for GitHub

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

Jump to bottom

Regional inlinepost #229

Merged
merged 31 commits into from
Mar 11, 2021
Merged

Regional inlinepost #229

merged 31 commits into from
Mar 11, 2021

Conversation

junwang-noaa
Copy link
Collaborator

@junwang-noaa junwang-noaa commented Jan 12, 2021 •
edited
Loading

Description

  1. Code changes have been added for inline post capability for regional applications (HAFS and LAM). The supported output grids are "regional_latlon" for HAFS, "rotated_latlon" and "lambert_comformal" for LAM.
  2. Remove the quilting constrain for forecast grid generation. The forecast grid is used for ESMF field generation in both quilting and coupling. This change allows forecast grid to be generated when quilting is .false.

Issue(s) addressed

Link the issues to be closed with this PR, whether in this repository, or in another repository.

  • fixes ufs-weather-model #259

Testing

Testing has been done on hera, orion and dell with regional_quilt and regional_quilt_hafs regression test tests and in high resolution LAM and HAFS tests.
Because of change item 2), the stretch nest tests need a input file INPUT/grid.nest02.tile7.nc to generate the forecast grid.

Dependencies

Related PRs:

  • ufs-weather-model PR #364.
  • post PR #255
  • This PR also requires a upp lib in hpc stack after post is merged.

@junwang-noaa junwang-noaa mentioned this pull request Jan 12, 2021
Merged
aerorahul
aerorahul reviewed Jan 12, 2021
View reviewed changes
io/post_regional.F90 Outdated
foundland = .false.
foundice = .false.

if(mype==0) print *,'dong haha', wrt_int_state%FBCount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am laughing!

@aerorahul
Copy link
Contributor

There are 2857 lines in io/post_regional.F90, of which more than 80% are reproduced (copied and pasted) from io/post_gfs.F90.
There is so much duplication in this code.

@DusanJovic-NOAA
Copy link
Collaborator

There are 2857 lines in io/post_regional.F90, of which more than 80% are reproduced (copied and pasted) from io/post_gfs.F90.
There is so much duplication in this code.

There is no fundamental difference between these two modules, apart from some minor grid related stuff, some error checking, rounding values to spval, debug prints and such thing. These two modules should be merged and in few cases those minor differences can be made by checking the grid type. I do not see the need to just copy entire module, and maintain virtually identical code, close to 3000 lines, in two places.

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Jan 12, 2021 via email

@DusanJovic-NOAA
Copy link
Collaborator

DusanJovic-NOAA commented Jan 12, 2021 •
edited
Loading

Well, one major change you did not mention is the "fillvalue" that global does not need while regional does need to identify undefined points and fill with post undefined value. Any idea how to sync those changes without adding if statement for "global" and "regional" around every field?
…
On Tue, Jan 12, 2021 at 5:06 PM Dusan Jovic @.***> wrote: There are 2857 lines in io/post_regional.F90, of which more than 80% are reproduced (copied and pasted) from io/post_gfs.F90. There is so much duplication in this code. There is no fundamental difference between these two modules, apart from some minor grid related stuff, some error checking, rounding values to spval, debug prints and such thing. These two modules should be merged and in few cases those minor differences can be made by checking the grid type. I do not see the need to just copy entire module, and maintain virtually identical code, close to 3000 lines, in two places. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#229 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI7D6TMV7ONQOVKCMND6WFDSZTBUJANCNFSM4V7YBHJA .

If the global field value is close to spval, shouldn't it also be rounded to spval, why is this specific to regional.
spval is 9.99e20 and small is 1.e-6. if difference between value and 9.99e20 is less that 1.e-6, it can be set to spval regardless of whether model is running in global or regional configuration.

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Jan 12, 2021 via email

@DusanJovic-NOAA
Copy link
Collaborator

Because global fields are valid on all the grid points, but regional output may be bigger than the computational domain so undefined values are filled on those points and they have to be refilled with post undefined value so that those points won't participate in the computation.

Exactly. That means all those if tests can be applied to both global and regional. Since global fields are 'never' close to spval (because they are valid everywhere), the if condition will never be true, and so they will remain as they are. Which means one module can be used for both global and regional configuration.

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Jan 13, 2021 via email

@DusanJovic-NOAA
Copy link
Collaborator

Why do we need to add extra computation for global fields?

Which computation? An if tests? Do we know how much slower 'regional' version is? A second or few seconds? Even if its's few seconds, this is running on write component which will not slow down the forecast tasks.

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Jan 13, 2021 via email

@DusanJovic-NOAA
Copy link
Collaborator

I am not proposing to compare regional configuration with or without fillvalue checks. I'm proposing to check the global configuration.
If the write component is slower than forecast we need to add more resources to it to always be faster, I understand that. The question is how much will those extra ifs slow down the write component?

The argument about regional using different physics than global is irrelevant here. What if two global configurations are using different physics? Are you going to create yet another copy, for those two different global physics configurations.

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Jan 13, 2021 via email

@DusanJovic-NOAA
Copy link
Collaborator

OK, if I understand correctly, the argument is: global fields do not need to check fillvalue as their values are valid., the code is clean. Why do we want to add extra code to check fillvalue for the global fields?

To avoid duplication of almost 3000 lines of code.

Do we know how much the extra code costs in terms of wall clock time?

How about setting spval to the same value as fillvalue, and avoid those checks even in regional? After all, both spval and fillvall are just some arbitrary large values. They do not have to be different.

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Jan 13, 2021 via email

@DusanJovic-NOAA
Copy link
Collaborator

Well, post is using one single spval for all undefined values for all the variables, while fields in the model have their own undefined values, unless we want to change the values in fv3 dycore.

Why can't those two undefined values be the same?

@junwang-noaa
Copy link
Collaborator Author

One is defined in dycore from GFDL, one is defined by post. We need to get all involved parties to discuss this.

@DusanJovic-NOAA
Copy link
Collaborator

One is defined in dycore from GFDL, one is defined by post. We need to get all involved parties to discuss this.

spval is set to 9.99e20 in set_postvars_regional:

https://github.com/junwang-noaa/fv3atm/blob/4801afd0cfe41fcd8f488c892c8b513d297927ee/io/post_regional.F90#L545

why can't it be set to the same value as fillvalue here? Why do we need to discuss this with 'all involved parties'?

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Jan 13, 2021 via email

@DusanJovic-NOAA
Copy link
Collaborator

Thanks for all of your questions. I'd suggest you read post code. Again, standalone post uses a single spval as undefined value, so inlinepost defines the same value used in standalone post. While different model fields have different filled values. To use a single undefined value in the model and post requires coordination from different repositories and additional post updates.

Why can't standalone post can use the spval set to 9.99e20 in the main program in the post (WRFPOST.f), while inline post use the spval set to fill value in the model. Any value of spval that can be used to identify missing values ( 9.99e20 or 1e30). Does it matter what the actual value is. After all this is used to set bitmap in the grib files, I do not think the actual value is written out in the post output, at least it should't be.

@DusanJovic-NOAA
Copy link
Collaborator

Even in the standalone post I see two different values for spval used:

WRFPOST.f:        spval = 9.9e10
WRFPOST.f:          spval = 9.99e20

depending on the file type of file format. I do not see why we can't set it to spval = fillvalue, in the write component.

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Jan 13, 2021 via email

@DusanJovic-NOAA
Copy link
Collaborator

fillvalue is different for different fields.

Then let's make them use the same value, and send that value to post as an indicator for missing data.

If you are so concerned about the cost of those fillvalue check in the global configuration you should be also, if not more, concerned about the cost in the regional configuration and try to avoid them, if they are not necessary.

WenMeng-NOAA
WenMeng-NOAA reviewed Feb 15, 2021
View reviewed changes
io/post_regional.F90 Outdated
@@ -2802,7 +2802,11 @@ subroutine set_postvars_regional(wrt_int_state,mpicomp,setvar_atmfile, &
do i=ista, iend
!assign sst
if (sm(i,j) /= 0.0 .and. ths(i,j) /= spval) then
if ((sice(i,j) >= 0.15) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@junwang-noaa A extra left bracket makes compiling failure.

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Feb 16, 2021 via email

@WenMeng-NOAA
Copy link
Contributor

WenMeng-NOAA commented Feb 16, 2021 via email

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Feb 16, 2021 via email

@junwang-noaa junwang-noaa mentioned this pull request Mar 4, 2021
Closed
WenMeng-NOAA
WenMeng-NOAA approved these changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@WenMeng-NOAA WenMeng-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes look good to me.

@junwang-noaa junwang-noaa merged commit 1c6d8fe into NOAA-EMC:develop Mar 11, 2021
AnningCheng-NOAA added a commit to AnningCheng-NOAA/fv3atm that referenced this pull request Mar 16, 2021
@AnningCheng-NOAA
Merge remote-tracking branch 'upstream/develop' into merra2
fab43ee 
* upstream/develop:
  Add missing OpenMP dependency to ccpp/data/CMakeLists.txt and ccpp/driver/CMakeLists.txt (NOAA-EMC#258)
  Regional inlinepost (NOAA-EMC#229)
@junwang-noaa junwang-noaa deleted the regional_inlinepost branch June 2, 2021 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aerorahul aerorahul aerorahul left review comments

@WenMeng-NOAA WenMeng-NOAA WenMeng-NOAA approved these changes

@DusanJovic-NOAA DusanJovic-NOAA DusanJovic-NOAA approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@junwang-noaa @aerorahul @DusanJovic-NOAA @WenMeng-NOAA