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

Bugfix for duplicated allocation and file closing for radar_rhv_opt = 2 #2078

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

mos3r3n
Copy link
Contributor

@mos3r3n mos3r3n commented Jul 10, 2024

Bugs fix of duplicated allocation and file close in radar_rhv_opt = 2.

TYPE: bug fix

KEYWORDS: radar DA

DESCRIPTION OF CHANGES:
Problem:
Duplicated allocation of avg_qws when choosing radar_rhv_opt = 2.
da_free_unit instead of da_get_unit for closing a file when radar_rhv_opt is equal to 2.

ISSUE: #2077

LIST OF MODIFIED FILES: list of changed files (use git diff --name-status master to get formatted list)
var/da/da_radar/da_get_innov_vector_radar.inc

@mos3r3n mos3r3n requested a review from a team as a code owner July 10, 2024 03:44
@mos3r3n mos3r3n changed the title Bugfix of duplicated allocation in radar DA when suing radar_rhv_opt = 2 Bugfix of duplicated allocation in radar DA when using radar_rhv_opt = 2 Jul 10, 2024
@weiwangncar
Copy link
Collaborator

The regression test results:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

@liujake liujake requested review from liujake and removed request for a team July 29, 2024 03:12
@liujake liujake requested a review from junmeiban July 29, 2024 03:13
@liujake
Copy link
Contributor

liujake commented Jul 29, 2024

@weiwangncar @islas Is it ok to merge this bugfix to 4.6.1?

@islas
Copy link
Collaborator

islas commented Jul 29, 2024

Removal of line 278 seems good, but the change to use da_free_unit seems to be separate from the rest of the PR. If these two lines are two different bug fixes it'd be best if they were split into different PRs.

@liujake liujake changed the title Bugfix of duplicated allocation in radar DA when using radar_rhv_opt = 2 Bugfix for duplicated allocation and file closing when using radar_rhv_opt = 2 Jul 29, 2024
@liujake liujake changed the title Bugfix for duplicated allocation and file closing when using radar_rhv_opt = 2 Bugfix for duplicated allocation and file closing Jul 29, 2024
@liujake
Copy link
Contributor

liujake commented Jul 29, 2024

Is it Ok to keep this single PR, but change the PR title (and content) to reflect there are 2 bugs fixed? Seems better to me to keep one single PR as changes are in one single file.

@islas
Copy link
Collaborator

islas commented Jul 29, 2024

I can see the argument for single PR if both are necessary to get radar_rhv_opt = 2 to work. In that case I think a more concise title to capture both would be something along the lines of "Bugfixes for radar DA when using radar_rhv_opt = 2" where the description notes both.

@mos3r3n
Copy link
Contributor Author

mos3r3n commented Jul 30, 2024

small bug fix of duplicated allocation in radar_rhv_opt = 2.

Hi, @islas. Both bugs are related to radar_rhv_opt = 2. I have updated the title and descriptions to make it clear. With both bug fixes, the radar_rhv_opt = 2 could work properly. Therefore, it cloud be better to merge both bugs in this PR.

@liujake liujake changed the title Bugfix for duplicated allocation and file closing Bugfix for duplicated allocation and file closing for radar_rhv_opt = 2 Aug 1, 2024
@liujake liujake merged commit 684f583 into wrf-model:release-v4.6.1 Aug 1, 2024
1 of 11 checks passed
@mos3r3n mos3r3n deleted the bugfix-radar_rhv_opt2 branch August 12, 2024 16:45
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.

5 participants