Skip to content

[codex] Add three-parameter double logistic selectivity - #1436

Draft
jimianelli wants to merge 1 commit into
NOAA-FIMS:mainfrom
jimianelli:codex/double-logistic3-selectivity
Draft

[codex] Add three-parameter double logistic selectivity#1436
jimianelli wants to merge 1 commit into
NOAA-FIMS:mainfrom
jimianelli:codex/double-logistic3-selectivity

Conversation

@jimianelli

@jimianelli jimianelli commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add a DoubleLogistic3Selectivity C++ functor implementing the 3-parameter double logistic form from jimianelli/selex.
  • Expose the new selectivity through the Rcpp module interface, R exports, and default parameter creation as DoubleLogistic3.
  • Add focused gtest and testthat coverage for the math helper, time-indexed evaluation, and R interface.

Validation

  • ./build/tests/gtest/fims_math_double_logistic3
  • ./build/tests/gtest/DoubleLogistic3_DoubleLogistic3Selectivity_Evaluate
  • R CMD INSTALL .
  • Rscript -e "library(FIMS); testthat::test_file('tests/testthat/test-rcpp-selectivity.R')"

Instructions for code reviewer

👋Hello reviewer👋, thank you for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete but feel free to skip over items that are not relevant!
  • See the GitHub documentation for how to comment on a PR to indicate where you have questions or changes are needed before approving the PR.
  • Please use standard conventional messages for both commit messages and comments
  • PR reviews are a great way to learn so feel free to share your tips and tricks. However, when suggesting changes to the PR that are optional please include nit: (for nitpicking) as the comment type. For example, nit: I prefer using a data.frame() instead of a matrix because ...
  • Engage with the developer. Make it clear when the PR is approved by selecting the approved status, and potentially commenting on the PR with something like This PR is now ready to be merged.

Checklist

  • The PR requests the appropriate base branch (dev for features and main for hot fixes)
  • The code is well-designed
  • The code is designed well for both users and developers
  • Code coverage remains high- [ ] Comments are clear, useful, and explain why instead of what
  • Code is appropriately documented (doxygen and roxygen)

@github-actions

Copy link
Copy Markdown
Contributor

🎨 Chore: code formatting workflow

Our automated workflows cannot run on forks because of permission issues, and thus, we ask that you run the following code locally and push any changes that are created to your feature branch. You will only be reminded of this once per PR. Thank you!

Format C++ code

  1. Install clang-format version 18.0.0
  2. Run the following command from the repository root:
    clang-format -i --style="{BasedOnStyle: Google, SortIncludes: false}" $(find ./inst/include ./src ./tests/gtest -name "*.hpp" -o -name "*.cpp")

Format R code

  1. Install {styler} and {roxygen2}
  2. Run the following commands in R from the repository root:
styler::style_pkg() # Style R code
roxygen2::roxygenise() # Update documentation
styler::style_pkg() # Style R code again
roxygen2::roxygenise() # Update documentation again
usethis::use_tidy_description() # Style DESCRIPTION file

Push changes

  1. Commit the formatting with a commit message of "Chore: format feature branch"
  2. Push to your fork

@codecov

codecov Bot commented Apr 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 41.29353% with 118 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.40%. Comparing base (b9d8f56) to head (d021fc7).
⚠️ Report is 61 commits behind head on main.

Files with missing lines Patch % Lines
...e/interface/rcpp/rcpp_objects/rcpp_selectivity.hpp 16.40% 107 Missing ⚠️
R/create_default_parameters.R 20.00% 8 Missing ⚠️
inst/include/common/fims_math.hpp 82.35% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1436      +/-   ##
==========================================
+ Coverage   82.97%   88.40%   +5.42%     
==========================================
  Files          54       85      +31     
  Lines        2214     8613    +6399     
  Branches      578      581       +3     
==========================================
+ Hits         1837     7614    +5777     
- Misses        277      965     +688     
+ Partials      100       34      -66     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kellijohnson-NOAA

Copy link
Copy Markdown
Contributor

@jimianelli sorry that it has taken me so long to get to this PR. I am wondering if you can outline the benefits of the three-parameter curve compared to the four-parameter curve that we already have implemented?

@jimianelli

Copy link
Copy Markdown
Contributor Author

@jimianelli sorry that it has taken me so long to get to this PR. I am wondering if you can outline the benefits of the three-parameter curve compared to the four-parameter curve that we already have implemented?

one-fewer parameter, and each remaining parameter has better interpretation for establishing prior distributions.

@jimianelli

Copy link
Copy Markdown
Contributor Author

also, unclear how this PR relates to the example app I set up in https://jimianelli.github.io/ebswp_fims/qmd/fims-implementation.html

@kellijohnson-NOAA

Copy link
Copy Markdown
Contributor

also, unclear how this PR relates to the example app I set up in https://jimianelli.github.io/ebswp_fims/qmd/fims-implementation.html

what do you mean by the above?

@jimianelli

Copy link
Copy Markdown
Contributor Author

also, unclear how this PR relates to the example app I set up in https://jimianelli.github.io/ebswp_fims/qmd/fims-implementation.html

what do you mean by the above?

just if you're seeking example applications, I could update it with the latest version, but if no interest that's fine too!

@kellijohnson-NOAA

Copy link
Copy Markdown
Contributor

@iantaylor-NOAA @ericward-noaa @MOshima-PIFSC @JonBrodziak @peterkuriyama-NOAA @alexjensen-NOAA @dgoethel-noaa @grantdadams @emilyliljestrand @timjmiller @erikhwilliams @nathanvaughan-NOAA thoughts on the 3-parameter double-logistic selectivity curve provided here? I am more than happy to update the PR and work with Jim to bring it in but I think we should decide (vote) as a group if we want to bring this in. Please use the thumbs up or thumbs down on this comment to let me know your preference.

@grantdadams

Copy link
Copy Markdown
Contributor

If it's an easy add, why not! If no stock is going to use it, maybe we prioritize something else.

@kellijohnson-NOAA

Copy link
Copy Markdown
Contributor

Adding it is pretty easy given Jim provided all the code. What I am trying to prevent is the ongoing maintenance of stuff that is not widely used. I do think it is a good start towards adding functionality in selectivity, which we desperately need but I am not sure if a curve that we already have, just parameterized in a different way, is what the team wants.

@iantaylor-NOAA

Copy link
Copy Markdown
Contributor

It would be nice to recommend a small set of flexible selectivity curves which can fit most users needs, but in order to do the research exploring the options, we need to have any good candidates built into an assessment model. I think that there's relatively little cost to including a larger set of selectivity options in FIMS and the benefits of having FIMS be the platform used to explore the options likely outweigh that cost.

I'll note that Mark Maunder also made a compelling argument in favor of a "Double Richards" selectivity at the 2026-04-07 UW Think Tank (starting at 29:22 in this recording https://www.youtube.com/watch?v=2qzct5y5LA0&t=1762s):
$$s_{a}=(1+e^{-\beta_{1}(a-\alpha_{1})})^{-1/\gamma_{1}} \times [1-(1+e^{-\beta_{2}(a-(\alpha_{1}+\alpha_{2}))})^{-1/\gamma_{2}}]$$
and described at https://www.populationassessment.com/marks-research/selectivity.

I would be interested in trying both.

I don't see a need to map over selectivity options from other models (e.g. double-normal from SS3) as long as the alternatives in FIMS are able to achieve similar-enough shapes and there's a compelling reason why they might be better.

@iantaylor-NOAA

Copy link
Copy Markdown
Contributor

I should add that if we add two parameterizations of the same selectivity option, or two similarly flexible selectivity options, we should commit to doing enough explorations to see which performs better (e.g. less parameter confounding, better posterior convergence, or whatever) and then deprecate or remove whichever options don't perform as well.

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