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 proportion female #654

Closed
wants to merge 6 commits into from
Closed

Conversation

kellijohnson-NOAA
Copy link
Contributor

What is the feature?

  • Reverts proportion_female to a single constant of 0.5

How have you implemented the solution?

  • Using a constant scalar.

Does the PR impact any other area of the project, maybe another repo?

  • The case studies need to be changed to not set proportion_female.

ChristineStawitz-NOAA and others added 5 commits July 25, 2024 08:32
add tests to ParameterVector

add tests for constructors

passing unit tests for ParameterVector

add more test cases for ParameterVector

add documentation
issue #170: refactor distributions_base

issue #581: add lognormal_lpdf

issue #573: add normal_lpdf

issue #585 add multinomial_lpmf

rename SIMULATE_F to FIMS_SIMULATE_F

rename REPORT_F to FIMS_REPORT_F

Co-authored-by: Matthew-Supernaw-NOAA <[email protected]>
Co-authored-by: Christine Stawitz - NOAA <[email protected]>
issue #388

convert tests to testthat format in:
   test-unit-rcpp-interface-variable-vector.R

update name of ParameterVector id to id_m

fix parameter order warning

update Rcpp class documentation

refactor operators

-collapses methods using S4 Group Generic Functions

-define methods for Ops, Math, and Summary

-update roxygen methods documentation

add fill_min and fill_max to ParameterVector

update parameter min/max default values
* issue #586

* link new densities to fleet

* modify interface

* fix bugs

* modify tests

* modify nll call in recruitment

* fix recruit eval nll

* add conditional is_na and comment out report

* move tmb pointer here

* fix warnings

* bug fix

* fix segfault issue

* fix nll summing bug

* add macros

* fix interface

* fix tests

* fix tmb distributions test - now passing

* delete tmb_distributions

* Add more normal tests cases -- failing still

* fix observed_values loop

* fix tests and add lognormal test and dimension check

* comment out dimension mismatch error check

---------

Co-authored-by: Matthew-Supernaw-NOAA <[email protected]>
Co-authored-by: Christine Stawitz - NOAA <[email protected]>
Co-authored-by: Cole-Monnahan-NOAA <[email protected]>
Co-authored-by: Chris Legault <[email protected]>
Co-authored-by: KyleShertzer-NOAA <[email protected]>
* issue #630

* update documentation style
Copy link
Contributor

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete!
  • For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review.
  • Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging.
  • Remember to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
  • PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include nit: (for nitpicking) before making the suggestion. For example, nit: I prefer using a data.frame() instead of a matrix because...
  • Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like This PR is now ready to be merged, no changes needed.

Checklist

  • The PR is requested to be merged into the appropriate branch (typically main)
  • The code is well-designed.
  • The functionality is good for the users of the code.
  • Any User Interface changes are sensible and look good.
  • The code isn’t more complex than it needs to be.
  • Code coverage remains high, indicating the new code is tested.
  • The developer used clear names for everything.
  • Comments are clear and useful, and mostly explain why instead of what.
  • Code is appropriately documented (doxygen and roxygen).

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 8.57143% with 64 lines in your changes missing coverage. Please review.

Project coverage is 66.06%. Comparing base (39d0743) to head (577a68b).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
inst/include/population_dynamics/fleet/fleet.hpp 0.00% 31 Missing ⚠️
...nst/include/distributions/functors/normal_lpdf.hpp 0.00% 18 Missing ⚠️
...nclude/distributions/functors/multinomial_lpmf.hpp 0.00% 13 Missing ⚠️
...distributions/functors/density_components_base.hpp 0.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (39d0743) and HEAD (577a68b). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (39d0743) HEAD (577a68b)
2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #654       +/-   ##
===========================================
- Coverage   78.88%   66.06%   -12.82%     
===========================================
  Files          36       19       -17     
  Lines        1975      725     -1250     
  Branches      141      196       +55     
===========================================
- Hits         1558      479     -1079     
+ Misses        374      165      -209     
- Partials       43       81       +38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kellijohnson-NOAA kellijohnson-NOAA force-pushed the fix-proportion_female branch 2 times, most recently from 0a82bcb to 8995cd6 Compare July 25, 2024 22:25
Users thought that they could set proportion_female but it was hard-
coded to 0.5 on the back end. Now, it is back to just being a single
value that is hard coded to 0.5. Tests have also changed back to 0.5.
This partially reverts PR #543. And, was talked about in
NOAA-FIMS/seaside-chats#7 and partially addresses #638.
@kellijohnson-NOAA
Copy link
Contributor Author

Now in cleanup_dev branch to be brought into dev.

@kellijohnson-NOAA kellijohnson-NOAA deleted the fix-proportion_female branch September 1, 2024 00:37
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.

4 participants