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

Issue with global mean weight for multiscale ensemble runs for ig > 1 plus changes to make code work for multiscale (bugs introduced in trunk). #570

Merged
merged 30 commits into from
May 30, 2023

Conversation

jderber-NOAA
Copy link
Contributor

@jderber-NOAA jderber-NOAA commented May 6, 2023

Description

While looking to optimize the use of multiscale ensembles, it was noted that the global mean wgt was not changed from the default (1.0) for all values of ig. The sum of the weights over all ig should be equal to 1 for the case being used. It was found that for all ig =1 the value of the weight should remain 1., but for all ig > 1 the value should be equal to zero. The original OU code had this in it. Somehow when being put into the GSI, the code doing this was removed.

Tests using the multiscale ensemble are starting and this needs to be fixed.

The only routine changed was init_mult_spc_wgts in apply_scaledepwgts.

Also, some cleanup of the routine in which the weights are set was included (e.g., 0 used for real zero, unused arrays removed, etc.)

When change was updated to the head of the trunk, two additional issues were found. The multiscale ensemble no longer worked and the regression test for the netcdf_fv3_regional no longer scaled between the low processor count and the high processor count. Both of these errors are present in the head of the trunk, not just in this branch.

The source of these two additional issues were found by running in debug mode and adding a lot of print statements. The solution to make it run the multiscale ensemble required a change to state_vectors.f90. Note a few small changes were also found to get the code to run completely in debug mode. To remove the processor scaling issue, a code section was removed from a later loop where it was put by the previous change (by me, sorry) and put in it's previous location before the cloud liquid water calculation in setuprad.f90.

After the previous two changes, the multiscale ensemble and the regression tests worked properly.

Fixes #569

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Weights produced by this routine were printed and only the global mean values for ig >1 were changed.

Regression tests were run. No differences were found (but I don't think any regression tests change the one routine changed by this update).

Results were compared to optimization tests (which also had this change, but a lot more) to ensure the results were similar (expected round off differences).

All tests were performed on Hera

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing tests pass with my changes
  • Any dependent changes have been merged and published

DUE DATE for this PR is 6/16/2023. If this PR is not merged into develop by this date, the PR will be closed and returned to the developer.

@jderber-NOAA jderber-NOAA self-assigned this May 6, 2023
@RussTreadon-NOAA RussTreadon-NOAA self-requested a review May 6, 2023 10:54
TingLei-NOAA
TingLei-NOAA previously approved these changes May 25, 2023
@shoyokota
Copy link
Collaborator

shoyokota commented May 25, 2023

I compared the following four tests of RRFS using Scale- and Variable-Dependent Localization (SDL: nsclgrp=2 and VDL: ngvarloc=2).

  1. The past develop branch before introducing the bug in control_vectors.f90 (1661c15)
  2. The current develop branch (726cc8d)
  3. 2 + modification of the bug in control_vectors.f90 only
  4. The branch of this PR (889d2e9)

As results, 2 was stopped by segmentation fault but 3 is completed. The result of 3 was slightly different from 1 because of the changes by PR#527. However, 4 was completely the same as 3, which indicates this PR does not affect the regional SDL/VDL except for the bug fix. All of these results are as we expected. I approve this PR.

shoyokota
shoyokota previously approved these changes May 25, 2023
@jderber-NOAA
Copy link
Contributor Author

jderber-NOAA commented May 25, 2023 via email

Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

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

One comment and one question.

@jderber-NOAA jderber-NOAA dismissed stale reviews from shoyokota and TingLei-NOAA via db1fd2d May 30, 2023 13:46
@RussTreadon-NOAA
Copy link
Contributor

@TingLei-NOAA and @shoyokota , thank you for reviewing the changes in this PR.

I asked John to remove a commented out line of code. He did so. Your reviews are now stale. I requested new reviews from each of you. Thank you for your prompt attention to this. We want to get John's bug fixes into develop sooner rather than later.

FYI, when you review code and leave comments, please remember to resolve your comments after the developer responds and you are satisfied with the response. I resolved your comments this morning to move this PR forward. I shouldn't do this since I don't know if John's response is satisfactory to you.

@TingLei-NOAA
Copy link
Contributor

@RussTreadon-NOAA I had given my approval of this PR. About "resolving "the conversation, I think, as we had discussed/verificed before, we have no "access/permission " to do this except for persons like you who have writing access to the trunk can do it. Is this github policy changed? I still failed to see that "resolving " button" .

@RussTreadon-NOAA
Copy link
Contributor

@TingLei-NOAA , if a reviewer does not have a button to click resolve, the reviewer should enter text in the comment window stating that the developer's response is satisfactory. This allows others to close a comment with certainty.

Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

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

Two peer review approvals given. Ctests run and documented.

@RussTreadon-NOAA
Copy link
Contributor

@jderber-NOAA , I clicked all the checkboxes except the first. Would you please confirm that you can check Bug fix (non-breaking change which fixes an issue) as complete?

I think you can do so as the PR author.

@jderber-NOAA
Copy link
Contributor Author

Box checked.

@jderber-NOAA
Copy link
Contributor Author

oops.

@RussTreadon-NOAA
Copy link
Contributor

Cross check with GSI Handling Review team. This PR is ready to merge!

@RussTreadon-NOAA RussTreadon-NOAA merged commit ac1a8cb into NOAA-EMC:develop May 30, 2023
@TingLei-NOAA
Copy link
Contributor

@TingLei-NOAA , if a reviewer does not have a button to click resolve, the reviewer should enter text in the comment window stating that the developer's response is satisfactory. This allows others to close a comment with certainty.

Thanks for this clarification!

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.

Issues with weights in multiscale ensemble analysis
4 participants