Skip to content

Fix/gamma lccdf v3 review#3292

Merged
syclik merged 3 commits intofix-gamma-lccdf-v3from
fix/gamma-lccdf-v3-review
Mar 12, 2026
Merged

Fix/gamma lccdf v3 review#3292
syclik merged 3 commits intofix-gamma-lccdf-v3from
fix/gamma-lccdf-v3-review

Conversation

@SteveBronder
Copy link
Collaborator

@SteveBronder SteveBronder commented Mar 11, 2026

Summary

  1. Removes the log gamma q and Q eval structs and instead uses std::pair
  2. Decreases precision default to sqrt(machine.error)
  3. Rewrote parts of log_q_gamma_cf to remove some state variables for in loop variables
  4. Used auto in functions where there are unrestricted template types. It was unclear to me if log_gamma_q_dgamma should ever take in autodiff variables. If so then the auto makes sense to me. But if they are only ever taking in or working with doubles then we should remove the templates entirely
  5. Uses std::optional<std::pair> to replace the bool ok{false}; in Q_eval.
  6. Replace *_dbl with *_val in functions with template parameters. If these functions accept autodiff types then *_dbl would imply the wrong type.
  7. Functions that can return back std::nullopt (empty std::optional) now early exit when the function can return std::nullopt

Checklist

  • Copyright holder: Simons Foundation

    The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
    - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
    - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

  • the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • dependencies checks pass, (make test-math-dependencies)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)
  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@syclik
Copy link
Member

syclik commented Mar 11, 2026

Awesome. I'll get it merged into the other branch today.

@SteveBronder SteveBronder mentioned this pull request Mar 11, 2026
4 tasks
@syclik
Copy link
Member

syclik commented Mar 12, 2026

@SteveBronder, I just made types explicit and I'm merging it.

  1. Replace *_dbl with *_val in functions with template parameters. If these functions accept autodiff types then *_dbl would imply the wrong type.

This isn't correct.

Although the functions are templated to be generic, the internal function can't be called under all uses of autodiff types. I had the same thought originally, but after working through the code, the code paths that use fvar don't go through the internal functions. I left the names as you wrote them.

@syclik syclik merged commit 15e00a1 into fix-gamma-lccdf-v3 Mar 12, 2026
@syclik
Copy link
Member

syclik commented Mar 12, 2026

(If there are problems on the original branch and testing due to my mistakes, I'll figure out how to correct it on the other PR.)

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.

2 participants