GitHub Issue NOAA-EMC/GSI#219. Improve minimization and fix bug in vqc #242
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks Russ. How do I add reviewers to the PR? I noticed you were able to
add yourself!
Su has looked at the differences and says they are OK.
John
…On Fri, Oct 22, 2021 at 12:01 PM RussTreadon-NOAA ***@***.***> wrote:
***@***.**** commented on this pull request.
Minor comments on first sweep. Will clone jderber-NOAA:master to look more
closely at the changes.
------------------------------
In regression/regression_var.sh
<#242 (comment)>:
> @@ -131,7 +131,7 @@ case $machine in
# On Hera, there are no scrubbers to remove old contents from stmp* directories.
# After completion of regression tests, will remove the regression test subdirecories
- export clean=".true."
+ export clean=".false."
We should revert this back to .true. since Hera does not automatically
script stmp.
------------------------------
In src/gsi/genqsat.f90
<#242 (comment)>:
> @@ -164,6 +162,7 @@ subroutine genqsat(qsat,tsen,prsl,lat2,lon2,nsig,ice,iderivative)
qsat(i,j,k) = max(qmin,qsat(i,j,k))
if(iderivative > 0)then
+! if(es <= esmax .and. iderivative == 2 .and. qsat(i,j,k) > qmin )then
Since this new line is commented out, do we need to retain it?
------------------------------
In src/gsi/observer.F90
<#242 (comment)>:
> @@ -174,10 +174,10 @@ subroutine guess_init_
endif
! Read output from previous min.
- if (l4dvar.and.jiterstart>1) then
- else
- ! If requested and if available, read guess solution.
- endif
+! if (l4dvar.and.jiterstart>1) then
Lines 177 to 180 look to be placeholders for future development. I'm OK
with removing them since they are commented out. Did GMAO add these lines?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#242 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASD2M5QJ7ZESISNQNJG3H3TUIGDEXANCNFSM5GQ2GYGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
XiujuanSu-NOAA
approved these changes
Oct 22, 2021
|
This is an option to consider. The question then becomes how to implement
this: (1) retain clean and add another flag to specify predetermined types
or degrees of file/directory removal, (2) change clean from a logical to
the flag mentioned in (1), or ...?
…On Tue, Oct 26, 2021 at 11:07 AM jderber-NOAA ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In regression/regression_var.sh
<#242 (comment)>:
> @@ -131,7 +131,7 @@ case $machine in
# On Hera, there are no scrubbers to remove old contents from stmp* directories.
# After completion of regression tests, will remove the regression test subdirecories
- export clean=".true."
+ export clean=".false."
Would be good to add option that allows everything to be deleted except
stdout and fort.*. This would get rid of a lot of the files and space, but
still allow a lot of debugging.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#242 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGNN63Y3VXUBC65JXMOAOB3UI274PANCNFSM5GQ2GYGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Russ,
I am trying it now with new options cleanmost and cleaninput. I am
thinking with cleanmost everything is removed except stdout and fort
files. With cleaninput all input files are removed. It would seem that it
would be good to run with cleaninput = true on all machines. That way
there are a lot less files on all machines.
Will take some lines of script to be specific as to what is removed, but in
the long range it could help.
I would put cleanmost to be the default on jet and hera and cleanmost to be
the default on the rest of the machines.
What do you think?
John
On Tue, Oct 26, 2021 at 11:14 AM RussTreadon-NOAA ***@***.***>
wrote:
… This is an option to consider. The question then becomes how to implement
this: (1) retain clean and add another flag to specify predetermined types
or degrees of file/directory removal, (2) change clean from a logical to
the flag mentioned in (1), or ...?
On Tue, Oct 26, 2021 at 11:07 AM jderber-NOAA ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In regression/regression_var.sh
> <#242 (comment)>:
>
> > @@ -131,7 +131,7 @@ case $machine in
>
> # On Hera, there are no scrubbers to remove old contents from stmp*
directories.
> # After completion of regression tests, will remove the regression test
subdirecories
> - export clean=".true."
> + export clean=".false."
>
> Would be good to add option that allows everything to be deleted except
> stdout and fort.*. This would get rid of a lot of the files and space,
but
> still allow a lot of debugging.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <#242 (comment)>, or
> unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AGNN63Y3VXUBC65JXMOAOB3UI274PANCNFSM5GQ2GYGQ
>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <
https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
>
> or Android
> <
https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub
>.
>
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#242 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASD2M5VLNETKNB22EPGTQODUI3ASZANCNFSM5GQ2GYGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
I mean cleaninput to be the default on the rest of the machines.
On Tue, Oct 26, 2021 at 11:35 AM John Derber - NOAA Affiliate <
***@***.***> wrote:
… Russ,
I am trying it now with new options cleanmost and cleaninput. I am
thinking with cleanmost everything is removed except stdout and fort
files. With cleaninput all input files are removed. It would seem that it
would be good to run with cleaninput = true on all machines. That way
there are a lot less files on all machines.
Will take some lines of script to be specific as to what is removed, but
in the long range it could help.
I would put cleanmost to be the default on jet and hera and cleanmost to
be the default on the rest of the machines.
What do you think?
John
On Tue, Oct 26, 2021 at 11:14 AM RussTreadon-NOAA <
***@***.***> wrote:
> This is an option to consider. The question then becomes how to implement
> this: (1) retain clean and add another flag to specify predetermined types
> or degrees of file/directory removal, (2) change clean from a logical to
> the flag mentioned in (1), or ...?
>
> On Tue, Oct 26, 2021 at 11:07 AM jderber-NOAA ***@***.***>
> wrote:
>
> > ***@***.**** commented on this pull request.
> > ------------------------------
> >
> > In regression/regression_var.sh
> > <#242 (comment)>:
> >
> > > @@ -131,7 +131,7 @@ case $machine in
> >
> > # On Hera, there are no scrubbers to remove old contents from stmp*
> directories.
> > # After completion of regression tests, will remove the regression test
> subdirecories
> > - export clean=".true."
> > + export clean=".false."
> >
> > Would be good to add option that allows everything to be deleted except
> > stdout and fort.*. This would get rid of a lot of the files and space,
> but
> > still allow a lot of debugging.
> >
> > —
> > You are receiving this because your review was requested.
> > Reply to this email directly, view it on GitHub
> > <#242 (comment)>, or
> > unsubscribe
> > <
> https://github.com/notifications/unsubscribe-auth/AGNN63Y3VXUBC65JXMOAOB3UI274PANCNFSM5GQ2GYGQ
> >
> > .
> > Triage notifications on the go with GitHub Mobile for iOS
> > <
> https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
> >
> > or Android
> > <
> https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub
> >.
> >
> >
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#242 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ASD2M5VLNETKNB22EPGTQODUI3ASZANCNFSM5GQ2GYGQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
|
|
cleaninput and cleanmost sound like good options. I'm thinking we may
want a third option, cleanall, to entirely remove the run directory. Let's
get Mike's take on this.
On Tue, Oct 26, 2021 at 11:39 AM jderber-NOAA ***@***.***>
wrote:
… I mean cleaninput to be the default on the rest of the machines.
On Tue, Oct 26, 2021 at 11:35 AM John Derber - NOAA Affiliate <
***@***.***> wrote:
> Russ,
>
> I am trying it now with new options cleanmost and cleaninput. I am
> thinking with cleanmost everything is removed except stdout and fort
> files. With cleaninput all input files are removed. It would seem that it
> would be good to run with cleaninput = true on all machines. That way
> there are a lot less files on all machines.
>
> Will take some lines of script to be specific as to what is removed, but
> in the long range it could help.
>
> I would put cleanmost to be the default on jet and hera and cleanmost to
> be the default on the rest of the machines.
>
> What do you think?
>
> John
>
> On Tue, Oct 26, 2021 at 11:14 AM RussTreadon-NOAA <
> ***@***.***> wrote:
>
>> This is an option to consider. The question then becomes how to
implement
>> this: (1) retain clean and add another flag to specify predetermined
types
>> or degrees of file/directory removal, (2) change clean from a logical to
>> the flag mentioned in (1), or ...?
>>
>> On Tue, Oct 26, 2021 at 11:07 AM jderber-NOAA ***@***.***>
>> wrote:
>>
>> > ***@***.**** commented on this pull request.
>> > ------------------------------
>> >
>> > In regression/regression_var.sh
>> > <#242 (comment)>:
>> >
>> > > @@ -131,7 +131,7 @@ case $machine in
>> >
>> > # On Hera, there are no scrubbers to remove old contents from stmp*
>> directories.
>> > # After completion of regression tests, will remove the regression
test
>> subdirecories
>> > - export clean=".true."
>> > + export clean=".false."
>> >
>> > Would be good to add option that allows everything to be deleted
except
>> > stdout and fort.*. This would get rid of a lot of the files and space,
>> but
>> > still allow a lot of debugging.
>> >
>> > —
>> > You are receiving this because your review was requested.
>> > Reply to this email directly, view it on GitHub
>> > <#242 (comment)>, or
>> > unsubscribe
>> > <
>>
https://github.com/notifications/unsubscribe-auth/AGNN63Y3VXUBC65JXMOAOB3UI274PANCNFSM5GQ2GYGQ
>> >
>> > .
>> > Triage notifications on the go with GitHub Mobile for iOS
>> > <
>>
https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
>> >
>> > or Android
>> > <
>>
https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub
>> >.
>> >
>> >
>>
>> —
>> You are receiving this because you authored the thread.
>> Reply to this email directly, view it on GitHub
>> <#242 (comment)>, or
>> unsubscribe
>> <
https://github.com/notifications/unsubscribe-auth/ASD2M5VLNETKNB22EPGTQODUI3ASZANCNFSM5GQ2GYGQ
>
>> .
>> Triage notifications on the go with GitHub Mobile for iOS
>> <
https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
>
>> or Android
>> <
https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub
>.
>>
>>
>
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#242 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGNN636ONSUEIL2SUD57VKLUI3DTJANCNFSM5GQ2GYGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Russ,
I agree with both retaining the remove all option and checking with Mike.
I was not removing the clean option which removes everything. So there
would be 3 options. I note that on hera and jet, clean=.true. or .false.
and on the other machines clean=false or true. So on other machines, the
periods are missing. Is this correct and just a characteristic of the
different machines?
John
On Tue, Oct 26, 2021 at 11:42 AM RussTreadon-NOAA ***@***.***>
wrote:
… cleaninput and cleanmost sound like good options. I'm thinking we may
want a third option, cleanall, to entirely remove the run directory. Let's
get Mike's take on this.
On Tue, Oct 26, 2021 at 11:39 AM jderber-NOAA ***@***.***>
wrote:
> I mean cleaninput to be the default on the rest of the machines.
>
> On Tue, Oct 26, 2021 at 11:35 AM John Derber - NOAA Affiliate <
> ***@***.***> wrote:
>
> > Russ,
> >
> > I am trying it now with new options cleanmost and cleaninput. I am
> > thinking with cleanmost everything is removed except stdout and fort
> > files. With cleaninput all input files are removed. It would seem that
it
> > would be good to run with cleaninput = true on all machines. That way
> > there are a lot less files on all machines.
> >
> > Will take some lines of script to be specific as to what is removed,
but
> > in the long range it could help.
> >
> > I would put cleanmost to be the default on jet and hera and cleanmost
to
> > be the default on the rest of the machines.
> >
> > What do you think?
> >
> > John
> >
> > On Tue, Oct 26, 2021 at 11:14 AM RussTreadon-NOAA <
> > ***@***.***> wrote:
> >
> >> This is an option to consider. The question then becomes how to
> implement
> >> this: (1) retain clean and add another flag to specify predetermined
> types
> >> or degrees of file/directory removal, (2) change clean from a logical
to
> >> the flag mentioned in (1), or ...?
> >>
> >> On Tue, Oct 26, 2021 at 11:07 AM jderber-NOAA ***@***.***>
> >> wrote:
> >>
> >> > ***@***.**** commented on this pull request.
> >> > ------------------------------
> >> >
> >> > In regression/regression_var.sh
> >> > <#242 (comment)>:
> >> >
> >> > > @@ -131,7 +131,7 @@ case $machine in
> >> >
> >> > # On Hera, there are no scrubbers to remove old contents from stmp*
> >> directories.
> >> > # After completion of regression tests, will remove the regression
> test
> >> subdirecories
> >> > - export clean=".true."
> >> > + export clean=".false."
> >> >
> >> > Would be good to add option that allows everything to be deleted
> except
> >> > stdout and fort.*. This would get rid of a lot of the files and
space,
> >> but
> >> > still allow a lot of debugging.
> >> >
> >> > —
> >> > You are receiving this because your review was requested.
> >> > Reply to this email directly, view it on GitHub
> >> > <#242 (comment)>,
or
> >> > unsubscribe
> >> > <
> >>
>
https://github.com/notifications/unsubscribe-auth/AGNN63Y3VXUBC65JXMOAOB3UI274PANCNFSM5GQ2GYGQ
> >> >
> >> > .
> >> > Triage notifications on the go with GitHub Mobile for iOS
> >> > <
> >>
>
https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
> >> >
> >> > or Android
> >> > <
> >>
>
https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub
> >> >.
> >> >
> >> >
> >>
> >> —
> >> You are receiving this because you authored the thread.
> >> Reply to this email directly, view it on GitHub
> >> <#242 (comment)>, or
> >> unsubscribe
> >> <
>
https://github.com/notifications/unsubscribe-auth/ASD2M5VLNETKNB22EPGTQODUI3ASZANCNFSM5GQ2GYGQ
> >
> >> .
> >> Triage notifications on the go with GitHub Mobile for iOS
> >> <
>
https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
> >
> >> or Android
> >> <
>
https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub
> >.
> >>
> >>
> >
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <#242 (comment)>, or
> unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AGNN636ONSUEIL2SUD57VKLUI3DTJANCNFSM5GQ2GYGQ
>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <
https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
>
> or Android
> <
https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub
>.
>
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#242 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASD2M5V6RMT5DEHLBKHBP2TUI3D7BANCNFSM5GQ2GYGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
@jderber-NOAA @RussTreadon-NOAA We can add different clean up options. With respect to debugging, all files are saved if the regression tests fail before a job completes. This doesn't help if you are expecting identical results and a test comes back with non-reproducible results. We can set the flag on which clean to use in regression_test and regression_test_enkf. If the reproducibility test fails, then keep stdout and the fort* files. Otherwise, clean up the regression test directory. |
|
Mike,
Thank you!
I am confused by your statement that " With respect to debugging, all files
are saved if the regression tests fail before a job completes." I am not
sure what this means. It seems to say that if the regression test fails
all files are saved. This is not true (at least on Hera). All files are
deleted regardless of passing or failing the regression test (at least on
Hera). This is what made me start thinking about this, because I kept
having to change clean to clean = .false. when the regression tests
failed. Perhaps the key part of the sentence is "before a job completes"?
Are you just talking about debugging when the jobs actually fail to end
properly, not when the results do not reproduce?
Also, I was thinking of doing several levels of cleaning.
1. clean=.true. everything is deleted. This is current default on jet and
Hera
2. cleaninput=.true. Input files and internal processor based files are
deleted. This could be default on all machines and would result in a lot
less files and disk space - but still allow a lot of debugging.
3. cleanmost=.true. All except fort and stdout files are deleted. This
could be new default on Jet and Hera. Not many files and not much disk
space, but allows one to do a lot of debugging.
We could probably add in some script that deletes most files (maybe keep
stdout so that we can compare run times, memory, etc.) if the regression
test is passed.
What do you think?
John
…On Wed, Oct 27, 2021 at 6:45 AM MichaelLueken-NOAA ***@***.***> wrote:
@jderber-NOAA <https://github.com/jderber-NOAA> @RussTreadon-NOAA
<https://github.com/RussTreadon-NOAA> We can add different clean up
options. With respect to debugging, all files are saved if the regression
tests fail before a job completes. This doesn't help if you are expecting
identical results and a test comes back with non-reproducible results. We
can set the flag on which clean to use in regression_test and
regression_test_enkf. If the reproducibility test fails, then keep stdout
and the fort* files. Otherwise, clean up the regression test directory.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#242 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASD2M5VIXVIMYA47A7UD7BTUI7J3XANCNFSM5GQ2GYGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
@jderber-NOAA @RussTreadon-NOAA By "with respect to debugging, all files are saved if the regression tests fail before a job completes," I meant that when the regression test returns a non-zero return code, then the directory will not be removed. So, as you say, "before the code completes," is the key part of the sentence. Currently, the regression tests aren't particularly useful for tracking reproducibility issues. The clean function was only added due to no scrubbing on Hera and Jet. Moving forward, we can rename clean to be cleanmost, and maintain this for only Hera and Jet. We can go into regression/regression_test.sh and force the removal of all input files and internal processor based files after the test is complete - without using clean, just remove these as default behavior. Where clean currently is, we would replace with cleanmost, and remove all files except for the stdout and fort.* files. |
CatherineThomas-NOAA
approved these changes
Nov 3, 2021
RussTreadon-NOAA
approved these changes
Nov 3, 2021
There was a problem hiding this comment.
I have completed my review. You will need to reduce the number of commits from five to one. I will include the steps to do this.
- In src/gsi/genqsat.f90, keeping line 165 commented is fine for now.
- In src/gsi/observer.F90, rather than commenting out lines 176-181, please remove these.
- In src/gsi/pcgsoi.f90, please add proper precision to real value on line 356 (0.5).
To correct the commit messages and remove the change in regression/regression_var.sh:
- git clone --recursive git@github.com:jderber-NOAA/GSI.git update
- cd update
- git reset --hard HEAD~1
- git reset --hard HEAD~1
- git reset --hard HEAD~1
- git reset --hard HEAD~1
- git reset --hard HEAD~1
- git remote add upstream git@github.com:NOAA-EMC/GSI.git
- git fetch upstream
- git merge upstream/master
- git checkout minimfixes
- git checkout master
- git merge --squash minimfixes
- git reset HEAD regression/regression_var.sh
- git checkout -- regression/regression_var.sh
- Please apply changes to src/gsi/observer.F90 and src/gsi/pcgsoi.f90
- git add src/gsi/observer.F90 src/gsi/pcgsoi.f90
git commit -m "GitHub Issue NOAA-EMC/GSI#219. Improve minimization and fix bug in vqc."- git push origin master --force
The regression tests were run and the results are the same as those that you noted in issue #219. The code was compiled in debug mode and no new warning messages were seen. The debug tests ran through to completion.
src/gsi/pcgsoi.f90
Outdated
| if(mype == 0)then | ||
| aindex=abs(dprod(3)/dprod(2)) | ||
| write(iout_iter,*) 'NL Index ',aindex | ||
| if(aindex > 0.5 .or. print_verbose) write(iout_iter,*) 'NL Values ', dprod(3),dprod(2) |
There was a problem hiding this comment.
Please add proper precision to real value, 0.5 (i.e., 0.5_r_kind).
| @@ -385,7 +383,6 @@ subroutine set_predictors_var | |||
|
|
|||
There was a problem hiding this comment.
While compiling in debug mode, ostats (line 378) and ostats_t (line 379) were identified as unused. Please remove unused variables.
| @@ -171,8 +171,6 @@ module crtm_interface | |||
| real(r_kind) , save ,allocatable,dimension(:,:) :: cloud_efr ! effective radius of cloud type in CRTM | |||
| real(r_kind) , save ,allocatable,dimension(:) :: cf ! effective radius of cloud type in CRTM | |||
| real(r_kind) , save ,allocatable,dimension(:) :: hwp_guess ! column total for each hydrometeor | |||
|
|
|||
| real(r_kind) , save ,allocatable,dimension(:,:,:,:) :: gesqsat ! qsat to calc rh for aero particle size estimate | |||
| real(r_kind) , save ,allocatable,dimension(:) :: table,table2,tablew ! GFDL saturation water vapor pressure tables | |||
| real(r_kind) , save ,allocatable,dimension(:) :: des2,desw ! GFDL saturation water vapor presure | |||
| real(r_kind) , save ,allocatable,dimension(:) :: lcloud4crtm_wk ! cloud info usage index for each channel | |||
There was a problem hiding this comment.
While compiling in debug mode - ges_tsen, ges_prsl, nfldsig (line 327), iderivative (line 348), and ice (line 350) - were identified as unused. Please remove unused variables.
| @@ -89,7 +89,7 @@ subroutine compute_derived(mype,init_pass) | |||
| use kinds, only: r_kind,i_kind | |||
| use jfunc, only: jiter,jiterstart,& | |||
| qoption,switch_on_derivatives,& | |||
| tendsflag,clip_supersaturation | |||
| tendsflag,superfact,clip_supersaturation | |||
There was a problem hiding this comment.
While compiling in debug mode, superfact was identified as being unused. Please remove unused variable.
src/gsi/setupw.f90
Outdated
| real(r_kind) residual,ressw,ress,val,vals,val2,valqc2,dudiff,dvdiff | ||
| real(r_kind) valqc,valu,valv,dx10,rlow,rhgh,drpx,prsfc,var_jb | ||
| real(r_kind) cg_t,cvar,wgt,term,rat_err2,qcgross | ||
| real(r_kind) residual,ressw,ress,vals,val2,valqc2,dudiff,dvdiff,rat_err2u |
There was a problem hiding this comment.
While compiling in debug mode, valqc2 was identified as unused. Please remove unused variable.
src/gsi/setupw.f90
Outdated
| real(r_kind) cg_t,cvar,wgt,term,rat_err2,qcgross | ||
| real(r_kind) residual,ressw,ress,vals,val2,valqc2,dudiff,dvdiff,rat_err2u | ||
| real(r_kind) valqc,valu,valv,dx10,rlow,rhgh,drpx,prsfc,var_jb,rat_err2v | ||
| real(r_kind) cg_t,cvar,wgt,term,rat_err2,qcgross,valqcu,valqcv |
There was a problem hiding this comment.
While compiling in debug mode, rat_err2 was identified as unused. Please remove unused variable.
|
Mike,
Thank you for looking at these changes and putting all of this together.
I followed your instructions and made the changes. The only changes I made
to your instructions is to remove the unused variables (from the routines
you noted) when making the changes to pcgsoi.f90 and observer.F90. I then
included these routines in the add. I think this is correct thing to do to
add these changes.
Please let me know if there is anything else that I need to do.
Best,
John
…On Thu, Nov 4, 2021 at 8:37 AM MichaelLueken-NOAA ***@***.***> wrote:
***@***.**** commented on this pull request.
After recompiling in debug mode, there were some unused variables. Please
remove the noted unused variables while updating your master.
------------------------------
In src/gsi/berror.f90
<#242 (comment)>:
> @@ -385,7 +383,6 @@ subroutine set_predictors_var
While compiling in debug mode, ostats (line 378) and ostats_t (line 379)
were identified as unused. Please remove unused variables.
------------------------------
In src/gsi/crtm_interface.f90
<#242 (comment)>:
> @@ -171,8 +171,6 @@ module crtm_interface
real(r_kind) , save ,allocatable,dimension(:,:) :: cloud_efr ! effective radius of cloud type in CRTM
real(r_kind) , save ,allocatable,dimension(:) :: cf ! effective radius of cloud type in CRTM
real(r_kind) , save ,allocatable,dimension(:) :: hwp_guess ! column total for each hydrometeor
-
- real(r_kind) , save ,allocatable,dimension(:,:,:,:) :: gesqsat ! qsat to calc rh for aero particle size estimate
real(r_kind) , save ,allocatable,dimension(:) :: table,table2,tablew ! GFDL saturation water vapor pressure tables
real(r_kind) , save ,allocatable,dimension(:) :: des2,desw ! GFDL saturation water vapor presure
real(r_kind) , save ,allocatable,dimension(:) :: lcloud4crtm_wk ! cloud info usage index for each channel
While compiling in debug mode - ges_tsen, ges_prsl, nfldsig (line 327),
iderivative (line 348), and ice (line 350) - were identified as unused.
Please remove unused variables.
------------------------------
In src/gsi/compute_derived.f90
<#242 (comment)>:
> @@ -89,7 +89,7 @@ subroutine compute_derived(mype,init_pass)
use kinds, only: r_kind,i_kind
use jfunc, only: jiter,jiterstart,&
qoption,switch_on_derivatives,&
- tendsflag,clip_supersaturation
+ tendsflag,superfact,clip_supersaturation
While compiling in debug mode, superfact was identified as being unused.
Please remove unused variable.
------------------------------
In src/gsi/setupw.f90
<#242 (comment)>:
> @@ -253,9 +253,9 @@ subroutine setupw(obsLL,odiagLL,lunin,mype,bwork,awork,nele,nobs,is,conv_diagsav
real(r_double) rstation_id
real(r_kind) qcu,qcv,trop5,tfact,fact
real(r_kind) scale,ratio,obserror,obserrlm
- real(r_kind) residual,ressw,ress,val,vals,val2,valqc2,dudiff,dvdiff
- real(r_kind) valqc,valu,valv,dx10,rlow,rhgh,drpx,prsfc,var_jb
- real(r_kind) cg_t,cvar,wgt,term,rat_err2,qcgross
+ real(r_kind) residual,ressw,ress,vals,val2,valqc2,dudiff,dvdiff,rat_err2u
While compiling in debug mode, valqc2 was identified as unused. Please
remove unused variable.
------------------------------
In src/gsi/setupw.f90
<#242 (comment)>:
> @@ -253,9 +253,9 @@ subroutine setupw(obsLL,odiagLL,lunin,mype,bwork,awork,nele,nobs,is,conv_diagsav
real(r_double) rstation_id
real(r_kind) qcu,qcv,trop5,tfact,fact
real(r_kind) scale,ratio,obserror,obserrlm
- real(r_kind) residual,ressw,ress,val,vals,val2,valqc2,dudiff,dvdiff
- real(r_kind) valqc,valu,valv,dx10,rlow,rhgh,drpx,prsfc,var_jb
- real(r_kind) cg_t,cvar,wgt,term,rat_err2,qcgross
+ real(r_kind) residual,ressw,ress,vals,val2,valqc2,dudiff,dvdiff,rat_err2u
+ real(r_kind) valqc,valu,valv,dx10,rlow,rhgh,drpx,prsfc,var_jb,rat_err2v
+ real(r_kind) cg_t,cvar,wgt,term,rat_err2,qcgross,valqcu,valqcv
While compiling in debug mode, rat_err2 was identified as unused. Please
remove unused variable.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#242 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASD2M5W37MCL5L2PS5K4GQLUKKEAJANCNFSM5GQ2GYGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
The due date for the review committee has past, with no feedback. I will now give final approval to these changes, then merge them to the authoritative master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change fixes bug in the vqc for winds. Also, in modifies the minimization algorithm so that it can detect nonlinearities. The minimization algorithm should produce the same results within round-off. Two new options are included. 1. To allow supersaturation (superfact). 2. To limit qobs to a value equal to superfact*qsat. The second option was included because there was a case found where there were many hurricane recon obs that produced q values greater than the supersaturated value with the analysis. This resulted in slow convergence. Both of these factors are set to values that will not impact the results (superfact=1.0, limitqobs = .false.
The resulting changes make resets very unlikely when the vqc option is used. Also, the nonlinearity diagnostic is very useful when determining minimization issues. Note it has indicated that the old variational quality control should not be used in any GSI runs.
A few small optimization and clean-ups were also included.