Skip to content

Conversation

@Crystal-szj
Copy link
Contributor

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

Description

Closes #279
Closes #280
Closes #283
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

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.

@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.

Copy link
Contributor

@yijianzeng yijianzeng left a comment

Choose a reason for hiding this comment

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

The variable name is suggested to change from Infiltration to effectivePrecip, which has been done.

@MostafaGomaa93 MostafaGomaa93 merged commit fe5fec3 into main Feb 26, 2025
1 check passed
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

4 participants