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

In filter_topo added extrapolation to halo points #571

Merged
merged 11 commits into from
Sep 17, 2021

Conversation

RatkoVasic-NOAA
Copy link
Contributor

This commit is related to the issue #105
Fields' geolat_c and geolon_c halo points are used to calculate dxc and dyc, but halo points are not initialized for regional standalone case. I added extrapolation for regional standalone similar way to regional nest and global application.

@GeorgeGayno-NOAA
Copy link
Collaborator

Thanks @RatkoVasic-NOAA. Does routine 'read_topo_file` have the same problem. What is in the halo region of arrays 'oro' and 'mask'?

@RatkoVasic-NOAA
Copy link
Contributor Author

Thanks @RatkoVasic-NOAA. Does routine 'read_topo_file` have the same problem. What is in the halo region of arrays 'oro' and 'mask'?

The only arrays (in regional standalone mode) that were using halo points are geolat_c and geolon_c. All other arrays are not using halo points.

@GeorgeGayno-NOAA
Copy link
Collaborator

@RatkoVasic-NOAA I tested your branch at b602411 on my canned test case. I compiled your branch on Hera with CMAKE_BUILD_TYPE equal to "Release" and "Debug". I got big differences along the halo. I will provide details.

@GeorgeGayno-NOAA
Copy link
Collaborator

Here are the differences between "Debug" and "Release" mode for my test case:

+ nccmp -dmqfS oro.C424.tile7.nc oro.C424.tile7.nc.ratko.intel.O3
Variable  Group Count     Sum AbsSum     Min     Max   Range     Mean  StdDev
orog_filt /        55 1.90058 1901.6 -239.36 239.794 479.154 0.034556 70.2504

I visualized the files. The differences are confined to the halo.

If you do not already know, you can change the compile type using this variable: -DCMAKE_BUILD_TYPE=Debug

@RatkoVasic-NOAA
Copy link
Contributor Author

I'll test it.

@GeorgeGayno-NOAA
Copy link
Collaborator

@RatkoVasic-NOAA I tried 1ec6689 on my canned case. The intel/release and intel/debug options gave identical results. The gnu/release option gives a different answer from the intel options. And the gnu/release and gnu/debug options give different answers. Differences are confined to the left edge of the grid. Here are the gnu differences:

$ nccmp -dmfqS oro.C424.tile7.nc.ratko.O3.gnu oro.C424.tile7.nc.ratko.debug.gnu
Variable  Group Count     Sum  AbsSum      Min     Max   Range    Mean  StdDev
orog_filt /        21 77.3102 97.5196 -7.94759 24.5957 32.5433 3.68144 7.90504

@RatkoVasic-NOAA
Copy link
Contributor Author

RatkoVasic-NOAA commented Aug 26, 2021 via email

@GeorgeGayno-NOAA
Copy link
Collaborator

Yes, this is a frustrating bug.

Also, I am writing a simple unit test for your new routine. I will check it in to your branch today.

to allow for using it in unit tests. Create simple unit
test for that routine.

Fixes ufs-community#105
@GeorgeGayno-NOAA
Copy link
Collaborator

@RatkoVasic-NOAA I added a unit test stub for you at f87f1f5.

@GeorgeGayno-NOAA
Copy link
Collaborator

@RatkoVasic-NOAA I see that your unit test was failing with a memory leak error. In your unit test, try deallocating "testdata" at the end of program execution.

@GeorgeGayno-NOAA
Copy link
Collaborator

It works!

@GeorgeGayno-NOAA
Copy link
Collaborator

@JeffBeck-NOAA may want to run a quick test with the regional workflow.

@RatkoVasic-NOAA is this ready to merge?

@RatkoVasic-NOAA
Copy link
Contributor Author

@RatkoVasic-NOAA is this ready to merge?

I replied by email, I guess it didn't go through.
Answer is yes, it is ready. Is there anything I should do more for this PR?

@GeorgeGayno-NOAA
Copy link
Collaborator

@RatkoVasic-NOAA is this ready to merge?

I replied by email, I guess it didn't go through.
Answer is yes, it is ready. Is there anything I should do more for this PR?

The 'grid_gen' regression tests will need to be run. Since results will likely change for the regional tests, new baseline files will need to be generated. @RatkoVasic-NOAA can you run the tests on Hera or WCOSS?

@GeorgeGayno-NOAA
Copy link
Collaborator

And merge to your branch the latest changes from 'develop'.

@GeorgeGayno-NOAA GeorgeGayno-NOAA self-requested a review September 17, 2021 20:31
@GeorgeGayno-NOAA
Copy link
Collaborator

@RatkoVasic-NOAA is this ready to merge?

I replied by email, I guess it didn't go through.
Answer is yes, it is ready. Is there anything I should do more for this PR?

The 'grid_gen' regression tests will need to be run. Since results will likely change for the regional tests, new baseline files will need to be generated. @RatkoVasic-NOAA can you run the tests on Hera or WCOSS?

Thanks. I have updated the baseline data on all machines.

@GeorgeGayno-NOAA GeorgeGayno-NOAA merged commit 69e009f into ufs-community:develop Sep 17, 2021
@RatkoVasic-NOAA RatkoVasic-NOAA deleted the filter_topo branch September 17, 2021 21:07
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.

2 participants