-
Notifications
You must be signed in to change notification settings - Fork 8
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 the test for population.CalculateRecruitment() #508
Fix the test for population.CalculateRecruitment() #508
Conversation
Instructions for code reviewerHello reviewer, thanks for taking the time to review this PR!
Checklist
|
3dbecd3
to
6191649
Compare
Codecov ReportAttention:
... and 28 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. Thanks @Bai-Li-NOAA for sorting all this so quickly.
Note: based on conversation during yesterday's meeting, now that I've completed the review, I've unassigned myself and assigned @Bai-Li-NOAA so she can choose how/whether to address the minor comments in the unresolved conversations. If I didn't have any comments, I would have just merged since I was the assignee. Once @Bai-Li-NOAA has resolved the conversations, she could either merge the PR or re-assign me if further input is needed. Please let me know if that workflow doesn't match how we're thinking of using PR assignments. |
42e66a2
to
9267058
Compare
test: add a test for testing population.CalculateRecruitment() fix: use EXPECTA_DOUBLE_EQ() to verifies that the two double values are approximately equal fix: fix the test for population.CalculateRecruitment()
9267058
to
dd1acf7
Compare
Thank you, @iantaylor-NOAA, for your review. I've incorporated your suggestions and updated the code. Additionally, I've resolved the merge conflicts. The workflow makes sense to me and I have merged the changes into the main branch. |
What is the feature?
This PR addresses issue #494 and fixes the test for population.CalculateRecruitment().
How have you implemented the solution?
Does the PR impact any other area of the project?
No.
How to test this change
The C++ test can be found here.
All GHA checks have passed.
Developer pre-PR checklist