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

Explain function Max_Rootdepth #176

Merged
merged 4 commits into from
Jun 17, 2024
Merged

Explain function Max_Rootdepth #176

merged 4 commits into from
Jun 17, 2024

Conversation

EntingTang
Copy link

@EntingTang EntingTang commented May 24, 2023

Description

Please add a description of the changes proposed in the pull request.

Update executable file

Please re-generate exe file if matlab codes are changed. About how to create an exe file, see exe readme.

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.
  • Model runs successfully.

closes #164

@EntingTang EntingTang requested review from SarahAlidoost, Yunfei-Wang1993 and Crystal-szj and removed request for SarahAlidoost May 24, 2023 14:08
Copy link
Contributor

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

@EntingTang Thanks for your effort. Please see my comments in the code.

src/Max_Rootdepth.m Outdated Show resolved Hide resolved
src/Max_Rootdepth.m Show resolved Hide resolved
src/Max_Rootdepth.m Outdated Show resolved Hide resolved
@SarahAlidoost
Copy link
Member

@EntingTang @Crystal-szj Hi, are you still working on this pull request? can you please fix the conflict and finalize the pull request? so it can be merged after that. If you need help with fixing the "merge conflict", let me know.

@Crystal-szj
Copy link
Contributor

Crystal-szj commented Jun 17, 2024

Hi @SarahAlidoost, I've fixed the conflict and compared the results using compare_git_branch_results.ipynb. The results are same. Thus, the changes for the Max_Rootdepth function are ready to merge into the main branch.

I'm wondering if it is beneficial to push the comparison results as well. Currently, I am encountering an issue with the passphrase on my local computer, so I haven't been able to push the results yet.

@SarahAlidoost
Copy link
Member

Hi @SarahAlidoost, I've fixed the conflict and compared the results using compare_git_branch_results.ipynb. The results are same. Thus, the changes for the Max_Rootdepth function are ready to merge into the main branch.

I'm wondering if it is beneficial to push the comparison results as well. Currently, I am encountering an issue with the passphrase on my local computer, so I haven't been able to push the results yet.

Thanks for fixing the pull request. no need to run the test notebook or re-generate the exe file because the changes are about adding comments only. Feel free to merge.

@Crystal-szj Crystal-szj merged commit ea1917b into main Jun 17, 2024
2 checks passed
@Crystal-szj Crystal-szj deleted the fix_issue164 branch June 17, 2024 16:11
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.

Refactoring Max_Rootdepth.m
4 participants