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

update for land IAU to ccpp-physics noahmp #12

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

tsga
Copy link

@tsga tsga commented Oct 30, 2024

This ensures consistency between the NoahMP component model and the UFS WM/ccpp physics Noahmp after the land IAU capability is added to the later. Specifically the noahmpdrv_init function has three parameters (Land IAU related DDTs) passed as optional, enabling the component model to use the noahmpdrv_init function without any more changes right now.

Dependent on pull request: ufs-community/ccpp-physics#222

@tsga
Copy link
Author

tsga commented Oct 30, 2024

Tagging relevant people here. Please add (tag) anyone you think should be aware.
@uturuncoglu, @barlage, @CoryMartin-NOAA, @ClaraDraper-NOAA, @CatherineThomas-NOAA, @junwang-noaa

Copy link

@CoryMartin-NOAA CoryMartin-NOAA left a comment

Choose a reason for hiding this comment

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

I don't know this code well but these changes seem correct from what I can tell

Copy link
Collaborator

@uturuncoglu uturuncoglu left a comment

Choose a reason for hiding this comment

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

I have just put couple of comment to clarify the implementation.

drivers/ccpp/noahmpdrv.meta Show resolved Hide resolved
@barlage
Copy link
Collaborator

barlage commented Oct 31, 2024

@tsga thanks for adding the IAU code here; I had a comment on that yesterday that apparently I didn't send; have you tried to compile and maybe run the land component reg test?

@uturuncoglu I assume there will need to be additions here

@tsga
Copy link
Author

tsga commented Oct 31, 2024

@tsga thanks for adding the IAU code here; I had a comment on that yesterday that apparently I didn't send; have you tried to compile and maybe run the land component reg test?

@uturuncoglu I assume there will need to be additions here

@barlage yes, the land component related reg tests passed after all the changes were made. I am now running the whole UWM regression tests.

And I have now added the CMakeLists.txt you mentioned to @uturuncoglu to noahmp repo as well (It is also part of the UWM repo, so I had modified it in the UWM repo--probably why the tests didn't fail).

@uturuncoglu
Copy link
Collaborator

@tsga Thanks. This looks fine. Just to be sure, did you run land component related RTs? If so, are they passing without any issue?

@tsga
Copy link
Author

tsga commented Nov 1, 2024

@tsga Thanks. This looks fine. Just to be sure, did you run land component related RTs? If so, are they passing without any issue?

@uturuncoglu yes, All of the regression tests listed in "rt.conf" pass. Yesterday, "datm_cdeps_lm4_gswp3_intel" did fail, but it passes now after I updated the LM4-driver repo. The new land iau test (control_c48_lnd_iau_intel) needs to be run with the -c switch because the baseline outputs are not yet copied in hera (and the other test platforms).

@barlage
Copy link
Collaborator

barlage commented Nov 4, 2024

@tsga what did you change in LM4? did you contact the LM4 group about those changes?

@tsga
Copy link
Author

tsga commented Nov 4, 2024

@tsga what did you change in LM4? did you contact the LM4 group about those changes?

No, I didn't change anything. I only needed to get my branch up to date with all the develop branch of UFSWM and its submodules, and everything works OK.

@barlage
Copy link
Collaborator

barlage commented Nov 4, 2024

@tsga what did you change in LM4? did you contact the LM4 group about those changes?

No, I didn't change anything. I only needed to get my branch up to date with all the develop branch of UFSWM and its submodules, and everything works OK.

OK, got it. I misinterpreted what you were saying.

Copy link

@ClaraDraper-NOAA ClaraDraper-NOAA left a comment

Choose a reason for hiding this comment

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

Looks OK to me - just one small comment. Thanks @tsga

@ClaraDraper-NOAA
Copy link

@tsga I reviewed this repo as this is where the science is done. Would you like to review the related PRs too?

@tsga
Copy link
Author

tsga commented Nov 7, 2024

@tsga I reviewed this repo as this is where the science is done. Would you like to review the related PRs too?

Thank you @ClaraDraper-NOAA. I think this is enough. The other two repo's will be reviewed by the code owners for dependency etc.

@tsga
Copy link
Author

tsga commented Nov 7, 2024

@uturuncoglu It seems the second test is failing because the files "lnd_iau_mod.F90" and "lnd_iau_mod.meta" are not copied to the noahmp directory. Can you please rerun the checks with the feature/lnd_iau branch contents and let me know if there is anything else to be done to merge this PR (the other related PRs are waiting for this to be complete).

@uturuncoglu
Copy link
Collaborator

@tsga Okay. Run it again. Let's see what happens.

CMakeLists.txt Outdated Show resolved Hide resolved
fix file path
@tsga
Copy link
Author

tsga commented Nov 12, 2024

@barlage Mike, now that the tests are all passed, can you please mark this PR as approved.

Copy link
Collaborator

@barlage barlage left a comment

Choose a reason for hiding this comment

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

Confirmed four files are identical to ccpp-physics

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