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

Gamma functions #67

Merged
merged 3 commits into from
Apr 11, 2024
Merged

Gamma functions #67

merged 3 commits into from
Apr 11, 2024

Conversation

eightysteele
Copy link
Contributor

Purpose

This PR adds native gamma function support with implementation references and sources (see the math.gamma namespace for details) and consolidates common math constants into a little namespace called math.utils.

Testing

All existing tests pass (bb test:clj, bb test:cljs).

Next steps

I would love a spot check and any feedback, happy to make changes. Otherwise it is my belief that this is good to go.

(+ (reduce (fn [sum [i l]] (+ sum (/ l (+ x i)))) 0.0 c)
0.99999999999999709182)))

(defn inv-gamma-1pm1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could potentially ^:no-doc this

(* (/ t x) (dec c))
(* x c))))))

(defn log-gamma-1p
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could potentially ^:no-doc this

@@ -25,15 +27,15 @@
(with-comparator (within 1e-11)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could eventually pull this out into it's own gamma-test namespace.

Copy link
Collaborator

@sritchie sritchie left a comment

Choose a reason for hiding this comment

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

Couple of tiny nitpicks then let's MERGE

[3 14.136097974741746]
[2 -59.59796035547549]
[1 57.15623566586292]]]
(+ (reduce (fn [sum [i l]] (+ sum (/ l (+ x i)))) 0.0 c)
Copy link
Collaborator

Choose a reason for hiding this comment

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

slight style thing, you could start your reduction with this value instead of 0.0 and save a level of nesting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, good catch—fixed.

0.166538611382291489501700795102105E+00
-0.420026350340952355290039348754298E-01
-0.655878071520253881077019515145390E+00]]
(if (< t 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

(neg? t)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice, done.

@eightysteele
Copy link
Contributor Author

eightysteele commented Apr 11, 2024

Fixed the nits, all checks passed—ready for merge.

@sritchie sritchie merged commit 495a6c7 into probcomp:main Apr 11, 2024
4 checks passed
@sritchie
Copy link
Collaborator

Boom, thanks for this, this is excellent work!

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