-
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 length tests #739
Fix length tests #739
Conversation
Instructions for code reviewerHello reviewer, thanks for taking the time to review this PR!
Checklist
|
@Andrea-Havron-NOAA it might be helpful for @Bai-Li-NOAA's review if you rebase this to current dev. Looks like you are missing a few commits. |
2b2956a
to
b8f205b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #739 +/- ##
=======================================
Coverage 68.71% 68.71%
=======================================
Files 44 44
Lines 4714 4714
Branches 197 197
=======================================
Hits 3239 3239
Misses 1428 1428
Partials 47 47 ☔ View full report in Codecov by Sentry. |
1911bd7
to
0be8c47
Compare
Thanks for the reminder @kellijohnson-NOAA! The branch has been rebased and is ready for review. |
tests/testthat/test-integration-fims-estimation-with-wrappers.R
Outdated
Show resolved
Hide resolved
tests/testthat/test-integration-fims-estimation-without-wrappers.R
Outdated
Show resolved
Hide resolved
} | ||
lengthcomp_nll <- lengthcomp_nll_fleet + lengthcomp_nll_survey | ||
|
||
expected_jnll <- rec_nll + index_nll + age_comp_nll + lengthcomp_nll | ||
jnll <- report[["jnll"]] | ||
|
||
expect_equal(report[["nll_components"]][1], rec_nll) |
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.
Non-blocking suggestion for this PR: Could we assign descriptive names to individual nll_components
? That way, we won't have to guess that the first element corresponds to recruitment nll and the second to fleet1 index nll.
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.
Great idea @Bai-Li-NOAA. I am just not entirely sure how to get this done because the elements present will change given the model structure. I am sure it is possible but we might have to think hard about how to do it correctly.
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.
Big thanks to @Andrea-Havron-NOAA for identifying the difference in dmultinom()
behavior between TMB and R. The fix to the tests looks great! I have a few minor edit suggestions and a potential non-blocking suggestion to refactor the interface in a separate PR. Let me know if you have any questions!
* update tests to use FIMS_dmultinom for length comps * remove TODO comments
e346af1
to
44fa66c
Compare
What is the feature?
How have you implemented the solution?
Does the PR impact any other area of the project, maybe another repo?