NCAR / icar

The Intermediate Complexity Atmospheric Research model (ICAR)
MIT License
72 stars 53 forks source link

bugfixes: halo_size and wind=1 smoothing #165

Closed scrasmussen closed 1 year ago

scrasmussen commented 1 year ago

TYPE: bugfix

KEYWORDS: halo exchange, smoothing, linear winds

SOURCE: Soren Rasmussen, NCAR

DESCRIPTION OF CHANGES: bugfixes:

  1. halo_size changed from module variable to exchangeable_t type variable. Previous behavior meant the value used by the last grid to set halo_size was used for every exchangeable object going forward. Procedures that had used halo_size now have the additional line halo_size = this%halo_size, which fixes behavior without this% word "pollution".
  2. wind=1 physics option was failing n-cores test due to smoothing behavior here. New larger exchange_obj variable used to cover the extra area needed for the smooth function to pass n-cores tests. This smoothing exxchange object can possibly can be reused other places if smoothing is causing issues. A new exchange subroutine has been added to perform north-south halo exchanges before performing east-west ones. This is to ensure halo regions from diagonal images are exchanged. The function is called exchange_directional_sync, this can be called something else if a different name is preferable.

TESTS CONDUCTED: Performed 32 and 36 image runs to plot the difference of the u-variable. The difference is shown in the following plot.

pr-fix-16hr

This is the difference after 16 hours for the develop branch. Thus this PR improves the n-core output.

develop-16hr

Performance runs where done using debugslow mode. The impact of the data movement and extra smooth calculations from using a larger array is shown in the following table comparing develop branch to pr-fix:

performance

NOTES: This PR fix does not produce n-core equality without two additional steps. The following two changes discussed would produce near equality after an hour run (there were two grid cells that would differed by -1.1920929e-07 and -5.9604645e-08 on the lowest 5 of 25 levels). The following two changes were only for testing.

  1. Comment out the smooth_array lines from here and here.
  2. This causes differences shown in the following plot. This is due to differing timesteps between images. Adding seconds = 45.0 to time_step.f90. This will now remove the following differences so the user can pass the n-core test. nosmooth-fix-ncore-difference-1hr

Checklist

Merging the PR depends on following checklist being completed. Add X between each of the square brackets if they are completed in the PR itself. If a bullet is not relevant to you, please comment on why below the bullet.

scrasmussen commented 1 year ago

P.S.

after 16 hours n-core differences start to be noticeable. But comparing at 24 hours and 1 week, the PR still is an improvement.

24 hour n-core diff

PR

pr-fix-24hr

Develop

develop-24hr

1 week n-core diff; note vmax change

1 week n-core diff with PR

pr-fix-1w

1 week n-core diff with develop

develop-1w
scrasmussen commented 1 year ago

@gutmann thanks for your input! Corrected the code and now this makes more sense. The value options%lt_options%stability_window_size, which is equal to 15, can be used to get good output without doing extra arithmetic to it. For the other one (nsmooth+2)*2 had to be used, which equals 14

And for comparison to the earlier plots to make sure that +1 to the nsmooth didn't break anything I ran for 24hrs.

PR 16hr n-cores diff

new-nsmooth-16hr

PR 24hr n-cores diff

new-nsmooth-24hr
gutmann commented 1 year ago

Great, we may need to investigate a little further since that wind field is not insignificantly different after 24hrs. If it keeps growing, that could be a problem still. I'll accept this PR though and Trude can test it in her longer simulations.