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 comment to smc pert unit & fix bug in stc pert #70

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

yuanxue2870
Copy link
Contributor

@yuanxue2870 yuanxue2870 commented Dec 11, 2023

In response to Issue (#71), two edits are made in this PR:

  1. Add a comment to "smc" perturbation unit, which will be helpful for new users to distinguish from actual "smc" unit commonly used in the model.
  2. Fix perturbation calculation in "stc" related part as pert*tfactor_state.

Copy link
Contributor

@tanyasmirnova tanyasmirnova 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 like it was an obvious bug in soil temperature perturbation. Glad that it was discovered and fixed.

Copy link
Contributor

@JeffBeck-NOAA JeffBeck-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 good! Thanks for fixing this bug!

@pjpegion
Copy link
Collaborator

Is there a PR at the ufs-weather-model level? And will this change a regression test baseline?

@yuanxue2870
Copy link
Contributor Author

Is there a PR at the ufs-weather-model level? And will this change a regression test baseline?

Thanks for the follow-up inquiries.

  • No, I did not put a PR at the ufs-weather-model. Do I need to create one? Please advise when you get a chance.
  • (Please feel free to correct me if I am wrong). No, there is no regression test perturbing "stc" based on what I have seen so far. Therefore, there will be no change in the regression test baseline.

@pjpegion
Copy link
Collaborator

yes, please create an github issue at https://github.com/ufs-community/ufs-weather-model/issues and issue a PR there for the update to the stochastic physics submodule.

@yuanxue2870
Copy link
Contributor Author

yes, please create an github issue at https://github.com/ufs-community/ufs-weather-model/issues and issue a PR there for the update to the stochastic physics submodule.

Done. Thanks.

Copy link
Collaborator

@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 good to me. Thanks Yuan!

@SamuelTrahanNOAA
Copy link
Contributor

This pull request's branch head does not match the ufs-weather-model stochastic_physics submodule HEAD.

Your branch has:

The submodule hash from ufs-community/ufs-weather-model#2043 is:

  • 5bf998e65f3aba07b99a7900b9ffc31537a5c1d9

Which is not in the repository.

Did you forget to push your latest changes?

@SamuelTrahanNOAA
Copy link
Contributor

I'm pinging @jkbk2004 so he is aware of the hash mismatch

@yuanxue2870
Copy link
Contributor Author

yuanxue2870 commented Dec 18, 2023

This pull request's branch head does not match the ufs-weather-model stochastic_physics submodule HEAD.

Your branch has:

The submodule hash from ufs-community/ufs-weather-model#2043 is:

  • 5bf998e65f3aba07b99a7900b9ffc31537a5c1d9

Which is not in the repository.

Did you forget to push your latest changes?

Hi Sam,

Thanks for the heads up! Sorry for the inconveniences.

As I am fairly new to Github, I think I may have some issues pushing my changes to the submodule at the ufs-weather-model level. I am not sure about the best practice to do this. If you could give me some advice, that would be greatly appreciated.

I first cloned/forked the lasted stochastics_physics and commit and push changes, the hash is 2afb1f4 at the stochastics_physics level.

Then I cloned/forked the lasted ufs-weather-model separately and commit and push my changes to the submodule again, which seems to change the hash to 5bf998. I did try to switch the .gitmodule to point to my branch, but does not seem to work.

Thanks for your help!

Best,
Yuan

@SamuelTrahanNOAA
Copy link
Contributor

@yuanxue2870 - This page will show you the changes in this pull request. Can you please look at them and tell us if these are the correct changes?

https://github.com/NOAA-PSL/stochastic_physics/pull/70/files

If the changes on that page are correct, I can use the 2afb1f4 version.

@yuanxue2870
Copy link
Contributor Author

@yuanxue2870 - This page will show you the changes in this pull request. Can you please look at them and tell us if these are the correct changes?

https://github.com/NOAA-PSL/stochastic_physics/pull/70/files

If the changes on that page are correct, I can use the 2afb1f4 version.

These are the exact changes. Please use the 2afb1f4 version.

Thanks,
Yuan

@SamuelTrahanNOAA
Copy link
Contributor

Excellent. I will begin testing the combined pull request now.

@jkbk2004
Copy link
Collaborator

@pjpegion all tests are done. we can move to merge this pr. can you merge this pr?

@pjpegion pjpegion merged commit 37e7405 into NOAA-PSL:master Dec 19, 2023
@jkbk2004
Copy link
Collaborator

Thanks, @pjpegion !

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.

7 participants