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

Allow ability for proportion female to vary by age #543

Merged
merged 8 commits into from
Feb 14, 2024

Conversation

ChristineStawitz-NOAA
Copy link
Contributor

What is the feature?

Allow proportion female to vary by age

How have you implemented the solution?

  • Added proportion female to Rcpp interface and made it possible to estimate it
  • Made proportion_female a fims::Vector of length nages
  • Modify population.hpp code to use vectorized proportion_female
  • Modified the population tests to use a vector for proportion female
    I did not add this to the estimation test, and have not yet tested if it can be estimated. I don't think we have a test data set to do that yet.

Does the PR impact any other area of the project?

Input is different because you can now add proportion_female as a field
Output has an extra parameter value if you estimate.

How to test this change

Developer pre-PR checklist

  • I relied on GitHub actions to 🧪 things for me while I sat on the 🛋️.

@ChristineStawitz-NOAA ChristineStawitz-NOAA linked an issue Feb 2, 2024 that may be closed by this pull request
1 task
Copy link
Contributor

github-actions bot commented Feb 2, 2024

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 Feb 2, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (70c6cc9) 75.28% compared to head (b83b9af) 75.33%.

Files Patch % Lines
...de/interface/rcpp/rcpp_objects/rcpp_population.hpp 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #543      +/-   ##
==========================================
+ Coverage   75.28%   75.33%   +0.04%     
==========================================
  Files          39       39              
  Lines        2080     2092      +12     
  Branches      140      141       +1     
==========================================
+ Hits         1566     1576      +10     
- Misses        471      473       +2     
  Partials       43       43              

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

@ChristineStawitz-NOAA
Copy link
Contributor Author

copied comment from other PR @k-doering-NOAA : @ChristineStawitz-NOAA I took a look and don't see any issues with this. I had a few questions, mostly for my understanding. The do not need to be resolved to merge, but answering them would help me :).

One thing that I'm not sure about is how much documentation this should have - perhaps at least saying how users would use this -999 on the description of this PR might be helpful? That way, it will give us something to start from once writing more formal documentation!

Copy link
Contributor

@Cole-Monnahan-NOAA Cole-Monnahan-NOAA left a comment

Choose a reason for hiding this comment

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

I was able to get the missing data to work in a case study. Coupled with the other reviews I think it's ready and will merge it in.

@ChristineStawitz-NOAA ChristineStawitz-NOAA merged commit 18f96a8 into main Feb 14, 2024
13 checks passed
@ChristineStawitz-NOAA ChristineStawitz-NOAA deleted the prop_female branch February 14, 2024 17:33
kellijohnson-NOAA added a commit that referenced this pull request Jul 25, 2024
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 added a commit that referenced this pull request Jul 25, 2024
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 added a commit that referenced this pull request Jul 25, 2024
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 added a commit that referenced this pull request Jul 25, 2024
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 added a commit that referenced this pull request Jul 25, 2024
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 added a commit that referenced this pull request Aug 31, 2024
proportion_female appeared to be estimable and/or specified by the user
but it was still hard coded to 0.5 for every age. This commit reverts
back part of PR #543 where the hard-coded value (not by age) was made
0.5 by age with a user interface.

Part of #638
kellijohnson-NOAA added a commit that referenced this pull request Sep 1, 2024
proportion_female appeared to be estimable and/or specified by the user
but it was still hard coded to 0.5 for every age. This commit reverts
back part of PR #543 where the hard-coded value (not by age) was made
0.5 by age with a user interface.

Part of #638
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.

[Feature]: Define proportion_female by age
4 participants