Skip to content

Dev dirchlet linear - #1516

Open
msupernaw wants to merge 30 commits into
mainfrom
dev-dirchlet-linear
Open

Dev dirchlet linear#1516
msupernaw wants to merge 30 commits into
mainfrom
dev-dirchlet-linear

Conversation

@msupernaw

@msupernaw msupernaw commented Jun 4, 2026 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

What is the feature?

  • Makes the Dirichlet linear multinomial available for the power user. Wrapper functions currently can't accommodate anything other than multinomial.

How have you implemented the solution?

  • Implemented the Dirichlet linear multinomial in the backend code.

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

  • no

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 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)

JonBrodziak and others added 30 commits March 24, 2026 15:33
@JonBrodziak
Upload gtest files to dev-dirchlet-linear. All gtests passed - see al…
a458fc2 
…l_tests_output.txt
@JonBrodziak
Remove comment on FIMS custom sum function
02e654e 
Removed comment about using the FIMS custom sum function.
@JonBrodziak
Remove fims_vector.hpp include from test file
dbbc7a4 
Removed unnecessary include for fims_vector.hpp.
@JonBrodziak
Upload modified gtest files to dev-dirchlet-linear. Gtest now uses 5 …
c67d685 
…test inputs for DM PMF read from csv created and verified with "Test_value_diric_multinom.R"
@JonBrodziak
Upload DM_lpmf
dae6c19
@JonBrodziak
upload DM_lpmf.hpp
e45758b
@JonBrodziak
Correct functor name
33654d9
@JonBrodziak
Upload rcpp_distribution.hpp from dev-diric-multinom
b4c4d35
@JonBrodziak
Delete uploaded SAP folder
9e44f2b
@JonBrodziak
Correct type in rcpp_distribution.hpp
8b8abc1
@JonBrodziak
Add DDirichletMultinomDistributionsInterface to fims_modules.hpp
723c2f4
@msupernaw
Fix Dirichlet-multinomial math and Rcpp interface
b7a37c2 
Use the global lgamma (::lgamma) in ddiric_multinom to avoid incorrect fims_math resolution, and update calls accordingly. Add fims_math include to the Dirichlet-multinomial functor and export the functor header from distributions.hpp. Fix Rcpp distribution interface bugs and naming/initialization issues: correct make_shared/copy-constructor types for the DDirichletMultinomial interface, initialize the underlying theta from initial_value_m, and update Rcpp bindings (method names, fields and expose theta). These changes address compilation/runtime errors and ensure the Dirichlet-multinomial distribution is properly registered and usable from R.
@msupernaw
Add DDirichletMultinom exports and wordlist
6c108b7 
Export DDirichletMultinomDistribution in NAMESPACE to make the new distribution available to package users. Also add DDirichletMultinomialDistributionsInterface to inst/WORDLIST so the identifier is recognized by the package wordlist (avoids spellcheck/lint issues).
@JonBrodziak
Add dirichlet_multinomial distribution support
463d86b 
Added 'dirichlet_multinomial' to available distributions and created a new function for it.
@JonBrodziak
Update DISCOVERY_TIMEOUT to 10 in CMakeLists.lst
94cf78b
@JonBrodziak
Merge branch 'dev-dirchlet-linear' of https://github.com/NOAA-FIMS/FIMS…
93a0a0d 
… into dev-dirchlet-linear
@JonBrodziak
Format distribution_formulas.R
a9c6dc1
@JonBrodziak
Clarify Dirichlet-multinomial log-probability details
27703bd 
Updated documentation @details to clarify the implementation of the Dirichlet-multinomial log-probability mass function.
@JonBrodziak
Update @file in dirichlet_multinomial_lmpf.hpp
7c1074e
@JonBrodziak
Update @file in rcpp_distribution.hpp
c7428cb
@JonBrodziak
Update @file in fims_modules.hpp
9137e3c
@JonBrodziak
Update comments to show filename
221d5eb
@JonBrodziak
Add filename to gtest and testthat template documentation
497c3bc
@JonBrodziak
Merge branch 'dev-dirchlet-linear' of https://github.com/NOAA-FIMS/FIMS…
15ae6c9 
… into dev-dirchlet-linear
@JonBrodziak
Update dirichlet_multinomial_lpmf.hpp
0743c7b 
Delete duplicate lines in @file
@JonBrodziak
Update testthat/helper-integration-tests*.R for DML
5a8a06c
@JonBrodziak
Upload testthat changes in R setup
c321b48
@JonBrodziak
Update on adding Dirichlet-Multinomial to R helper tests and simple p…
541c86f 
…roposed fix to switch between composition distribution types in "fix_helper_setup_R1.docx". More general solution recommended for multiple composition types and other submodel components.
@JonBrodziak
Updates to helper-integration-tests-setup-function.R and helper-integ…
af62061 
…ration-tests-validation.R to support Dirichlet-Multinomial distribution.
@JonBrodziak
Add solution for switch from multinomial to dirichlet in fix_*_FIMS_w…
d420ee0 
…ithout_helper.R
@kellijohnson-NOAA kellijohnson-NOAA linked an issue Jun 9, 2026 that may be closed by this pull request
[FEATURE] Dirichlet-multinomial likelihood for composition data #112
Open
@kellijohnson-NOAA

kellijohnson-NOAA commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

This branch needs to be rebased to main. I have done it locally and can push for you if you want but I wasn't sure what was best for you given there are two of you working on this branch. With the rebase, I wiped out all changes to tests/gtest/CmakeLists.txt and then added the Dirchlet-mulitnomial test back in after near the fimsMath tests rather than at the end of the file. Second, why do you have the naming convention of ddiric_multinom rather than spelling it out in full? There are quite a few abbreviations like this that I think should be written out in full for comprehension purposes. Additionally, if the words are written out in full then the changes to the WORDLIST file will be slim to none. There was some code added to the R directory but it is not complete and should probably be removed.
Once these things are fixed, I will assign Andrea to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kellijohnson-NOAA kellijohnson-NOAA Awaiting requested review from kellijohnson-NOAA
@Andrea-Havron-NOAA Andrea-Havron-NOAA Awaiting requested review from Andrea-Havron-NOAA

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE] Dirichlet-multinomial likelihood for composition data

3 participants

@msupernaw @kellijohnson-NOAA @JonBrodziak