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

fix issue #163 #174

Merged
merged 5 commits into from
Jul 13, 2023
Merged

fix issue #163 #174

merged 5 commits into from
Jul 13, 2023

Conversation

DanyangYu
Copy link
Contributor

@DanyangYu DanyangYu commented May 24, 2023

Description

Refator the Root_property fuction

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 #163

@DanyangYu DanyangYu requested review from geek-yang and EntingTang and removed request for BSchilperoort and SarahAlidoost May 24, 2023 14:15
@geek-yang
Copy link
Member

It would be nice to have a descriptive title of your PR.

src/ebal.m Show resolved Hide resolved
Copy link
Member

@geek-yang geek-yang left a comment

Choose a reason for hiding this comment

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

Nice work! I left some comments and address them. Thanks a lot 😄 !

Copy link

@EntingTang EntingTang left a comment

Choose a reason for hiding this comment

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

Good to add references, explain the if-statements and loops, make codes clean.
Possible improvement: meaning of input and output variables should be explained

src/Root_properties.m Outdated Show resolved Hide resolved
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.

Nice to add the reference and explanations for the function and variables 👍. Please see my comments in the code.

src/Root_properties.m Outdated Show resolved Hide resolved
src/Root_properties.m Outdated Show resolved Hide resolved
src/ebal.m Show resolved Hide resolved
@DanyangYu DanyangYu closed this Jul 13, 2023
@DanyangYu DanyangYu reopened this Jul 13, 2023
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.

Thanks Danyang, nice work!

@DanyangYu DanyangYu merged commit 8271075 into main Jul 13, 2023
1 check failed
@DanyangYu DanyangYu deleted the fix_163 branch July 13, 2023 09:33
@DanyangYu DanyangYu restored the fix_163 branch July 13, 2023 09:34
@SarahAlidoost
Copy link
Member

@DanyangYu and @Crystal-szj Thank you for working on the pull request. I noticed that you approved and merged the pull request while the checks were not green. This will reduce the quality of the codes in the main branch. For future pull requests, please make sure that the checks are green before approving and merging them.

@SarahAlidoost SarahAlidoost deleted the fix_163 branch July 17, 2023 08:24
@DanyangYu
Copy link
Contributor Author

Got it

@Crystal-szj Crystal-szj mentioned this pull request Jul 17, 2023
9 tasks
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 Root_properties.m
5 participants