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

Fix initialization of ??_corner flags #42

Closed
wants to merge 2 commits into from
Closed

Fix initialization of ??_corner flags #42

wants to merge 2 commits into from

Conversation

ofuhrer
Copy link
Contributor

@ofuhrer ofuhrer commented May 7, 2020 •
edited
Loading

This fixes issue #41 where of the ??_corner flags in init_grid() in tools/fv_grid_tools.F90 are being read before setting them. The initialization of ??_corner has been moved to tools/fv_grid_tools.F90.

Side note to @bensonr and @lharris4: It would be great to have some regression tests integrated into GitHub in order to make sure that such PR's actually pass regression.

Thanks to @rheacangeo for the bug report!

ofuhrer added 2 commits May 7, 2020 10:09
@ofuhrer
Removed setting of ??_corner flags
5d47b9b 
Will move this to fv_grid_tools.F90 in next commit.
@ofuhrer
Set ??_corner flags in init_grid()
aaede94 
This has been moved from grid_utils_init() in model/fv_grid_utils.F90
@lharris4
Copy link
Contributor

lharris4 commented May 7, 2020 via email

@underwoo
Copy link
Member

underwoo commented May 7, 2020

Hi, Oliver. Thanks. We are working on a set of regression tests but I don't think there is a good way to run them on GitHub. I know the MOM6 team has a way to do it but it took them years to set it all up. Lucas

@lharris4, the MOM6 team has two types of tests. The small tests (mainly a few builds with small tests to verify the executable works and reproduces) are run on Travis CI. After the Travis CI tests pass, they bring the updates onto our internal GitLab instance where they run longer tests on Gaea. If you would like to look into doing this with GFDL_atmos_cubed_sphere, we can talk with the MOM group and help get you setup. However, outside collaborators will not be able to see the results of these tests.

We are also in the process of investigating the use of one or more of the cloud providers (e.g. AWS, Google Compute, Azure, etc.) to run full regression tests. Last year, @thomas-robinson and myself showed it was possible to run regression tests on one of the cloud providers. With the help of others, we are working to acquire funding to implement the full solution. This will allow us to use GitHub Actions configured to use the cloud provider to run both quick verification (build, style, etc) tests, and when desired full regression tests.

If you would like more information on this, we can put a meeting together to answer any questions you and others may have.

@ofuhrer
Copy link
Contributor Author

ofuhrer commented May 7, 2020

Thanks @lharris4 and @underwoo for your answers.

We've used GitHub integration for TravisCI, CircleCI and Jenkins and it works great. The advantage of making test results also visible to contributors who make a PR, is that you can push the responsibility of getting tests to pass to the developers which takes a big burden off the shoulders of the code maintainers. This is how a PR would look like...

image

At least for MOM5 it seems that the TravisCI test results are visible to outside users.

We would be more than happy to help with this as well, if there's anything you can see us do.

@nbren12
Copy link

nbren12 commented May 7, 2020

I would just add that even a simple test usingTravis/github/CircleCIs free tier would be an incredibly useful tool. This practice is ubiquitous in the open source community for a reason, it greatly amplifies the trust between maintainers and contributors, and can greatly accelerate development.

The great thing about CI systems is you can add complexity incrementally. At Vulcan, we started with bit-for-bit regression tests of the C48 model (which took approx 2 week to set up, including learning how to build the model in docker), and now we have much more sophisticated system, but that doesn't need to happen overnight.

@bensonr bensonr mentioned this pull request May 8, 2020
Closed
@bensonr
Copy link
Contributor

bensonr commented May 8, 2020

Thanks @nbren12 @ofuhrer for your suggestions.

We use a combination of Travis and GH actions for FMS CI today and are building containers for a tier of real data tests with GFDL models. We have a plan to use these same concepts to build a CI system for the dycore. Anything you can offer to accelerate the process will be appreciated.

I'd like to move this conversation to issue #43 with collaboration occurring via GH, direct email, and video conferences as needed.

@bensonr bensonr requested review from lharris4 and bensonr May 12, 2020 14:49
@bensonr
Copy link
Contributor

bensonr commented May 12, 2020

@nbren12 can you perform a code review on the updates in this PR - thanks

@nbren12
Copy link

nbren12 commented May 13, 2020

@bensonr I wish I could. I’m not really an expert on this part of the code base, just got pulled in for the CI discussion. Maybe @rheacangeo can take a look?

@rheacangeo
Copy link

rheacangeo commented May 14, 2020 •
edited
Loading

The code changes look good to me, but it does change 'the answer', so it's a question of whether that change is acceptable. Also whether it causes other consequences. e.g. there is a bunch of edge and corner handling of area_c after the code block that currently isn't executed in master because the corner variables aren't set yet. It doesn't look like any of them have the same impact, but it's possible for something there to be 'compensating' for the corner code not getting executed. But more likely, I would guess this routine was written assuming the corner variables had already been set and it's just a bug fix . In our regression tests using C12, this changes files:
"atmos_dt_atmos.tile*.nc"
"sfc_dt_atmos.tile*.nc"
and all of the tile restart files
"RESTART/*.tile*.nc"
All the variables I looked at have small, but not negligible differences. e.g. I attached difference plots of UGRD at 500mb (~0.4 m/s) and RH at 850m (~0.2%) differences.
U_500_diff
RH_850_diff

But with such a small number of cells the impacts are likely exaggerated. It might make sense to check out diffs on a more realistic resolution.

@ofuhrer
Copy link
Contributor Author

ofuhrer commented Jul 9, 2020

Not sure where this is at. I guess the change does cause meteorological differences and thus should go through some sort of testing on GFDL side.

@lharris4
Copy link
Contributor

lharris4 commented Jul 10, 2020 via email

@bensonr
Copy link
Contributor

bensonr commented Jul 14, 2020

Lucas - let me know when you've finished your analysis by performing a formal review. Once done, I will merge it in.

@lharris4
Copy link
Contributor

lharris4 commented Jul 15, 2020 via email

@lharris4
Copy link
Contributor

lharris4 commented Jul 15, 2020 via email

@ofuhrer
Copy link
Contributor Author

ofuhrer commented Jul 15, 2020

Hi Lucas,
All good, just note that in the current implementation these flags are being read before they are actually being set (see original Issue #41 .

@lharris4
Copy link
Contributor

lharris4 commented Jul 15, 2020 via email

@DusanJovic-NOAA DusanJovic-NOAA mentioned this pull request Jun 21, 2021
Merged
6 tasks
@bensonr
Copy link
Contributor

bensonr commented Jun 25, 2021

Fixed via PR #113 and cherry-picked to master.

@bensonr bensonr closed this Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lharris4 lharris4 Awaiting requested review from lharris4

@bensonr bensonr Awaiting requested review from bensonr

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

Successfully merging this pull request may close these issues.
6 participants
@ofuhrer @lharris4 @underwoo @nbren12 @bensonr @rheacangeo