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

Correct Snow precipitation #282

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

Crystal-szj
Copy link
Contributor

@Crystal-szj Crystal-szj commented Feb 17, 2025

Description

Closes #279
Closes #280
Relates #281

Checklist

  • Add a reference to related issues.
  • @mentions of the person or team responsible for reviewing proposed changes.
  • This pull request has a descriptive title.
  • Code is written according to the guidelines.
  • The checks by MISS_HIT style checker and
    linter
    , below the pull request, are
    successful (green).
  • Documentation is available.
  • Add changes to the changelog file under section Unreleased.
  • Model runs successfully, see tests.
  • Ask a meinatainer to re-generate exe file if matlab codes are changed. About how to create an exe file, see exe readme.

@Crystal-szj Crystal-szj mentioned this pull request Feb 17, 2025
9 tasks
@Crystal-szj Crystal-szj self-assigned this Feb 17, 2025
@Crystal-szj Crystal-szj added the bug Something isn't working label Feb 17, 2025
Copy link
Contributor Author

@Crystal-szj Crystal-szj left a comment

Choose a reason for hiding this comment

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

Hi @MostafaGomaa93, Thanks for your efforts.
Could you please add the definitions of input and output variables in the functions related to your changes (add references if possible)? Additionally, for the new-added variables, let's follow the MATLAB Guidelines 2.0 about naming conventions

% Apply the surface boundary condition called for by BoundaryCondition.NBCh
%%%%%% Apply the surface boundary condition called for by BoundaryCondition.NBCh %%%%%%
Precip = ForcingData.Precip_msr(KT); % total precipitation (liquid + snow)
R_Dunn = ForcingData.Runoff_Dunn(KT); % Dunnian runoff (calculated in +io/loadForcingData file)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
R_Dunn = ForcingData.Runoff_Dunn(KT); % Dunnian runoff (calculated in +io/loadForcingData file)
runoffDunn = ForcingData.runoffDunn(KT); % Dunnian runoff (calculated in +io/loadForcingData file)

Copy link
Contributor Author

@Crystal-szj Crystal-szj left a comment

Choose a reason for hiding this comment

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

See my comment below.

ForcingData.Precip_snow = Precip_snow;
ForcingData.Precip_snowAccum = Precip_snowAccum;
ForcingData.infiltration = infiltration;
ForcingData.runoffDunn = runoffDunn;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the ForcingData.runoffDunn the same as ForcingData.runoffDunnian? Do you mean ForcingData.runoffDunnian here? Please double-check which variable you want to save here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they are the same. The difference is that ForcingData.runoffDunn is a 1D array but ForcingData.runoffDunnian is a time-series array. and the ForcingData.runoffDunn is needed in BMI

Copy link
Contributor

Choose a reason for hiding this comment

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

ForcingData.runoffDunn is needed in BMI

Should we make it explicitly clear then? something like ForcingData.runoffDunn_BMI ?

@@ -46,6 +46,6 @@
else
L_ts = L(n);
SH = 0.1200 * Constants.c_a * (SoilVariables.T(n) - ForcingData.Ta_msr(KT)) / r_a_SOIL(KT);
RHS(n) = RHS(n) + 100 * Rn_SOIL(KT) / 1800 - Constants.RHOL * L_ts * Evap - SH + Constants.RHOL * Constants.c_L * (ForcingData.Ta_msr(KT) * Precip(KT) + BoundaryCondition.DSTOR0 * SoilVariables.T(n) / Delt_t); % J cm-2 s-1
RHS(n) = RHS(n) + 100 * Rn_SOIL(KT) / 1800 - Constants.RHOL * L_ts * Evap - SH + Constants.RHOL * Constants.c_L * (ForcingData.Ta_msr(KT) * ForcingData.infiltration + BoundaryCondition.DSTOR0 * SoilVariables.T(n) / Delt_t); % J cm-2 s-1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yijianzeng @Yunfei-Wang1993 @Lianyu-Yu Could you please help explain what these variables mean, their units and references (if possible)?Evap, Delt_t, r_a_SOIL, Rn_SOIL, RHS, L, L_ts, SH, BoundaryCondition.DSTOR0, SoilVariables.T?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @Crystal-szj, i had a look at the original STEMMUS code, and suggest the following:
Evap is Evaporation (g cm^2 s^-1)
Delt_t is the time step (here, it is 1800s)
r_a_SOIL is soil surface aerodynamic resistance (s cm^-1)
Rn_SOIL is net radiation reaching the soil surface
L is the latent heat of vaporization, Line47 'L_ts=L(n)' could be added by Lianyu.

@Crystal-szj
Copy link
Contributor Author

Thank you, @MostafaGomaa93, for your efforts. I tested your branch and it works well. I ran 10 timesteps against main and correct_precip_snow_backup to compare the results with the groundwater option turned off. The results are exactly the same.

Hi @yijianzeng, we need your review to unlock the merge. Could you please review the changes Mostafa made to correct the calculation of precipitation/snow when the soil temperatures are below zero?

@yijianzeng
Copy link
Contributor

Thank you, @MostafaGomaa93, for your efforts. I tested your branch, and it works well. I ran 10 timesteps against main and correct_precip_snow_backup to compare the results with the groundwater option turned off. The results are the same.

@Crystal-szj, does it make sense to post the test results after you, Mostafa, or others have done this step?

@Crystal-szj
Copy link
Contributor Author

Thank you, @MostafaGomaa93, for your efforts. I tested your branch, and it works well. I ran 10 timesteps against main and correct_precip_snow_backup to compare the results with the groundwater option turned off. The results are the same.

@Crystal-szj, does it make sense to post the test results after you, Mostafa, or others have done this step?

Hi @yijianzeng, thanks for your suggestions. I have added the comparison here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants