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

Add PM2.5 DA for RRFS-SD #448

Merged

Conversation

hongli-wang
Copy link

@hongli-wang hongli-wang commented Nov 14, 2022

No description provided.

Copy link
Collaborator

@guoqing-noaa guoqing-noaa left a comment

Choose a reason for hiding this comment

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

Suggest to merge the following pairs:

gsiparm.anl.sh               gsiparm.anl_sd.sh
exregional_run_analysis.sh   exregional_run_analysis_sd.sh

JREGIONAL_RUN_ANA            JREGIONAL_RUN_ANAL_SD

netcdf_diag=.true.,binary_diag=.false.,
l_obsprvdiag=${l_obsprvdiag},
/
netcdf_diag=${netcdf_diag},binary_diag=${binary_diag},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line 24 should be inside the &SETUP block

Copy link
Author

Choose a reason for hiding this comment

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

Line 24 will be removed.

hzscl=${bkgerr_hzscl},
bw=0.,fstat=.true.,
/
usenewgfsberror=${usenewgfsberror},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line 34 should be inside the &BKGERR block

Copy link
Author

Choose a reason for hiding this comment

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

Line 34 will be removed.

fv3sar_bg_opt=${fv3lam_bg_type},
/
l_hyb_ens=${ifhyb},
uv_hyb_ens=.true.,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line 59-63: remove line 62, replace line 59 with line 63

Copy link
Author

Choose a reason for hiding this comment

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

The lines will be removed.

@@ -0,0 +1,92 @@
gsi_namelist="
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest to combine gsiparm.anl_sd.sh into gsiparm.anl.sh

Copy link
Author

Choose a reason for hiding this comment

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

This is doable. PM2.5 DA is just a 3DVar run with a static B generated for aerosol DA. The current design only need one parameter DO_SDDACYCLE to switch among tasks. Othwise more parameters will be needed to pass to the merged scripts.

ush/set_rrfs_config.sh Show resolved Hide resolved
ush/templates/FV3LAM_wflow.xml Show resolved Hide resolved
@hongli-wang
Copy link
Author

Suggest to merge the following pairs:

gsiparm.anl.sh               gsiparm.anl_sd.sh
exregional_run_analysis.sh   exregional_run_analysis_sd.sh

JREGIONAL_RUN_ANA            JREGIONAL_RUN_ANAL_SD

I thought about this. The current implementation is the most efficient way to run a separate 3DVAR to produce the smoke and dust traces for RRFS-SD. I have no problem merging them to make the modification fellow the standard of the regional workflow.

@hongli-wang
Copy link
Author

Suggest to merge the following pairs:

gsiparm.anl.sh               gsiparm.anl_sd.sh
exregional_run_analysis.sh   exregional_run_analysis_sd.sh

JREGIONAL_RUN_ANA            JREGIONAL_RUN_ANAL_SD

@guoqing-noaa I had merged the files as you suggested and pushed back to my branch. Should I create. a new PR from my latest commit? Thanks.

@hongli-wang
Copy link
Author

@guoqing-noaa I just merged my commits and forced pushed back to my branch. This PR should work. Thanks.

@hongli-wang
Copy link
Author

Suggest to merge the following pairs:

gsiparm.anl.sh               gsiparm.anl_sd.sh
exregional_run_analysis.sh   exregional_run_analysis_sd.sh

JREGIONAL_RUN_ANA            JREGIONAL_RUN_ANAL_SD

Hi Guoqing,

Could you please review this PR since I had revised the scripts after your comments. There were conflicts shown however, I had rebased my code and pushed back today.

Thanks,
Hongli

@@ -198,6 +199,8 @@ OBS_INPUT::
i_gsdqc=2,
/
&CHEM
laeroana_fv3smoke=.true.,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to set the default value of laeroana_fv3smoke to .false. so that this will not affect other parts when not doing chem DA?

Suggest to add a variable in config files and then we can write as follows:
laeroana_fv3smoke=${laeroana_fv3smoke}
You could reference line 21, 22 in this file.

   netcdf_diag=${netcdf_diag},binary_diag=${binary_diag},
   l_obsprvdiag=${l_obsprvdiag},

Copy link
Author

Choose a reason for hiding this comment

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

I can update this. The issue is that the base of GSL GSI is a few months behind my GSI, so the GSL GSI can NOT the two parameters, which has to be deleted as you commented in a next review.

I can add a variable in the config file, but to make the GSL GSI work, the parameters have to be deleted when running a regular meteorological DA.

sed '/berror_fv3_cmaq_regional/d' gsiparm.anl.1 > gsiparm.anl
rm -fr gsiparm.anl.sd gsiparm.anl.1
fi

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to update GSI first and then we don't need this temporary workaround?

Copy link
Author

Choose a reason for hiding this comment

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

This requires GSL GSI is rebased to the latest EMC GSI development branch. If not, we have to delete the parameters related to SMOKE DA to make the meteorological DA run. That is one reason I added a separated *sh to produce gsiparm.anl for SD DA in my previous PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to update GSL GSI with those two namelist parameters according to the EMC/develop branch? Don't need to add the full functionality at the moment, but only to add placeholders so that GSL GSI be able to proceed with the new namelist.

Copy link
Author

Choose a reason for hiding this comment

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

@guoqing-noaa
I think it would be good I keep the lines in my personal application or add the two parameters in GSL/GSI. I will delete the lines in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! GSL/GSI has to be updated with these new namelist parameters any way. If you do it now (even just putting a namelist placeholder), it will save some future work, :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, I just checked the latest GSL/GSI (the feature/rrfs_dev branch). It already included the following two parameters:

berror_fv3_cmaq_regional
laeroana_fv3smoke

Copy link
Author

Choose a reason for hiding this comment

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

@guoqing-noaa
That is great. My current test is based base_e, which doesn't accept the two namelist parameters. I may use the lastest GSL GSI rrfs_dev in my next test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry that although berror_fv3_cmaq_regional is already there and we also have the laeroana_fv3smoke variable in chemmod.f90, laeroana_fv3smoke is not added in gsimod.f90 yet. I believe it was added only in your own GSI copy at the moment. Could you create a PR to update GSL/GSI with this? (otherwise it will stop the normal RRFS workflow). Thanks!

scripts/exregional_run_analysis.sh Show resolved Hide resolved
scripts/exregional_run_analysis.sh Show resolved Hide resolved
i_use_2mQ4B=2,
i_use_2mT4B=1,
i_use_2mQ4B=${ii_use_2mq4b},
i_use_2mT4B=${ii_use_2mt4b},
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be consistent, we can use ${i_use_2mQ4B} and ${i_use_2mT4B} in the config file.
Similar to line 160-161:

   readin_localization=${readin_localization},
   ens_fast_read=${ens_fast_read},

Copy link
Author

Choose a reason for hiding this comment

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

I will change.

@@ -348,6 +348,8 @@ niter2=50
lread_obs_save=.false.
lread_obs_skip=.false.
if_model_dbz=.false.
ii_use_2mq4b=2
ii_use_2mt4b=1
Copy link
Collaborator

Choose a reason for hiding this comment

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

See a previous comment on this. Also suggest to put them in the config files.

Copy link
Author

Choose a reason for hiding this comment

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

This is default for base_E. I will change.

Copy link
Author

Choose a reason for hiding this comment

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

@guoqing-noaa

Correction: this is analysis type dependent, needs to be here. My GSI doesn't support (2,1) options for the two parameters.

Copy link
Author

Choose a reason for hiding this comment

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

@guoqing-noaa
The two parameters had been defined in config file:
i_use_2mQ4B=0
i_use_2mT4B=0

They need to assign different values for meteorological DA or SD DA, so they are also in this shell script.

scripts/exregional_run_analysis.sh Outdated Show resolved Hide resolved
i_use_2mQ4B=2,
i_use_2mT4B=1,
i_use_2mQ4B=${i_use_2mq4b},
i_use_2mT4B=${i_use_2mt4b},
Copy link
Collaborator

Choose a reason for hiding this comment

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

${i_use_2mq4b} and ${i_use_2mt4b}
should be ${i_use_2mQ4B} and ${i_use_2mT4B}
They are case-sensitive.

@@ -348,6 +348,8 @@ niter2=50
lread_obs_save=.false.
lread_obs_skip=.false.
if_model_dbz=.false.
i_use_2mQ4B=2
i_use_2mT4b=1
Copy link
Collaborator

Choose a reason for hiding this comment

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

i_use_2mT4b --> i_use_2mT4B

Copy link
Author

Choose a reason for hiding this comment

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

@guoqing-noaa fixed. thanks.

nummem=0
beta1_inv=0.0
i_use_2mQ4b=0
i_use_2mT4b=0
Copy link
Collaborator

Choose a reason for hiding this comment

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

655 and 656 change to:
i_use_2mQ4B=0
i_use_2mT4B=0

Copy link
Author

Choose a reason for hiding this comment

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

@guoqing-noaa Fixed. Thanks.

@hongli-wang hongli-wang force-pushed the feature/RRFS_dev1 branch 2 times, most recently from 2066cf1 to 64ba50f Compare January 19, 2023 00:04
@@ -198,6 +199,8 @@ OBS_INPUT::
i_gsdqc=2,
/
&CHEM
laeroana_fv3smoke=${laeroana_fv3smoke},
berror_fv3_cmaq_regional=${berror_fv3_cmaq_regional}.,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just found that there is an extra dot before the comma (line 203).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants