-
Notifications
You must be signed in to change notification settings - Fork 9
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
Reorganize test data #741
Reorganize test data #741
Conversation
Instructions for code reviewerHello reviewer, thanks for taking the time to review this PR!
Checklist
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #741 +/- ##
==========================================
+ Coverage 68.71% 68.92% +0.21%
==========================================
Files 44 44
Lines 4714 4714
Branches 197 197
==========================================
+ Hits 3239 3249 +10
+ Misses 1428 1418 -10
Partials 47 47 ☔ View full report in Codecov by Sentry. |
85a2cc2
to
6b025eb
Compare
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.
Thanks @Bai-Li-NOAA for these changes. I think this work is a great movement towards cleaning up the codebase as a whole and making things more consistent. I added two commits that should be squashed when you are ready. I do have a question about the sdr_report and the use of @.
tests/testthat/test-integration-fims-estimation-with-wrappers.R
Outdated
Show resolved
Hide resolved
validate_fims( | ||
report = agecomp_fit@report, | ||
sdr = agecomp_fit@estimates, | ||
sdr_report = agecomp_fit@estimates, |
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.
How come the sdr_report is the estimates table?
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.
I’ve updated the tests to use get_report() and get_estimates() when working with the wrapper functions.
tests/testthat/test-integration-fims-estimation-with-wrappers.R
Outdated
Show resolved
Hide resolved
tests/testthat/test-integration-fims-estimation-with-wrappers.R
Outdated
Show resolved
Hide resolved
tests/testthat/test-integration-fims-estimation-with-wrappers.R
Outdated
Show resolved
Hide resolved
tests/testthat/test-integration-fims-estimation-with-wrappers.R
Outdated
Show resolved
Hide resolved
* use test fixtures to properly generate multiple combinations of available data * add integration tests to validate handling of missing vlaues in the datasets * clean helper function validate_fims() and remove duplicated code * add TODO tasks to estimate recruitment log_devs later
c980334
to
78532f8
Compare
@kellijohnson-NOAA, I have addressed the great suggestions! I wanted to note a few additional changes that may need further review:
|
@Bai-Li-NOAA great job. Please rebase and merge into dev whenever you see fit. |
What is the feature?
How have you implemented the solution?
Does the PR impact any other area of the project, maybe another repo?