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

Fix/fengsha moist #253

Merged
merged 9 commits into from
Sep 25, 2023
Merged

Fix/fengsha moist #253

merged 9 commits into from
Sep 25, 2023

Conversation

bbakernoaa
Copy link

Found an issue with the gravimetric soil moisture units used within the FECAN soil moisture correction. This fixes that as well as adds a few knobs to be able to tune the soil moisture and drylimit within the dust scheme.

@bbakernoaa bbakernoaa requested a review from a team as a code owner September 15, 2023 19:29
@bbakernoaa
Copy link
Author

bbakernoaa commented Sep 19, 2023

Anything I need to do for this?

@mathomp4
Copy link
Member

@bbakernoaa A couple things:

  1. Can you add an entry to the CHANGELOG.md file in the root directory explaining your changes.
  2. We require a label (either 0-diff or non-zero-diff) to let the @GEOS-ESM/aerosol-team team know if this would affect results. Now, technically I suppose it is non-zero-diff if you run Fengsha but then we (GMAO) don't run that I think, so...I guess you can label it zero-diff? (Since, in a way, it is to us.)

@bbakernoaa
Copy link
Author

@mathomp4 I'll try to add something to the changelog and the label this morning. Thanks

add changes for fengsha
@bbakernoaa
Copy link
Author

@mathomp4 I added a changelog but if I'm suppose to add a label here in the PR I don't have permission to do so.

CHANGELOG.md Outdated Show resolved Hide resolved
@mathomp4 mathomp4 added 0 diff The changes in this pull request have verified to be zero-diff with the target branch. Non 0-diff The changes in this pull request are non-zero-diff and removed Non 0-diff The changes in this pull request are non-zero-diff labels Sep 19, 2023
@mathomp4
Copy link
Member

@mathomp4 I added a changelog but if I'm suppose to add a label here in the PR I don't have permission to do so.

Ah. Interesting. I didn't know you needed write access to make a label! Well, I've done it for you. I'll keep it as zero-diff since we don't run Fengsha.

But, I'll be careful and do a test to make sure!

@mathomp4
Copy link
Member

Yep. This seems to be zero-diff at least how I know how to run GOCART (which I guess isn't Fengsha!)

@bbakernoaa
Copy link
Author

Good this should only effect FENGSHA and not the Ginoux or K14 schemes

@tclune
Copy link
Contributor

tclune commented Sep 22, 2023

@amdasilva @vbuchard Our NOAA colleagues would like to know when they can expect this to be merged.

@mathomp4
Copy link
Member

@bbakernoaa We are confused by:

- Add `soil_moisture_factor` to the DU2G_GridComp.rc and DU2G_GridCompMod.F90 files for FENGSHA
- Add `soil_drylimit_factor` to the DU2G_GridComp.rc and DU2G_GridCompMod.F90 files for FENGSHA

First, do you mean DU2G_instance_DU.rc? Second, on your branch, I see no updates to the FENGSHA bits at the bottom:

https://github.com/bbakernoaa/GOCART/blob/fix/fengsha_moist/ESMF/GOCART2G_GridComp/DU2G_GridComp/DU2G_instance_DU.rc

there is only:

# FENGSHA settings
alpha: 0.3
gamma: 1.3
vertical_to_horizontal_flux_ratio_limit: 2.e-04

@bbakernoaa
Copy link
Author

@mathomp4 Yes sorry. It will need to be in the DU2G_instance_DU.rc file. I used the same names as the k14 so I didn't want to duplicate it in the rc file. They do the same things as in the k14 scheme.

Could you please advise on how we should do that?

@mathomp4
Copy link
Member

@mathomp4 Yes sorry. It will need to be in the DU2G_instance_DU.rc file. I used the same names as the k14 so I didn't want to duplicate it in the rc file. They do the same things as in the k14 scheme.

Could you please advise on how we should do that?

Ohhh. Okay. I see. I didn't notice they were above and not commented out! Well, I suppose this works then.

adding soil_drylimit_factor for the fengsha scheme. Note that soil_moisture_factor was also used in the k14 scheme.  Not duplicating here.
@bbakernoaa
Copy link
Author

@mathomp4 I just added the soil_drylimit_factor to the .rc file as this one wasn't duplicated

@vbuchard
Copy link
Contributor

@mathomp4 I just added the soil_drylimit_factor to the .rc file as this one wasn't duplicated

Can you please update the changelog accordingly (with the right filename) before I can approve/merge the PR. With all these updates, it looks ok to me. Thanks @mathomp4 for doing the testing.

@bbakernoaa
Copy link
Author

@vbuchard I modified it. Please let me know if that works.

Copy link
Contributor

@vbuchard vbuchard left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thanks

@vbuchard vbuchard merged commit f7b34e1 into GEOS-ESM:develop Sep 25, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants