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

Shared tests for various distributions #35

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Conversation

sritchie
Copy link
Collaborator

@sritchie sritchie commented Aug 30, 2023

@zane , let me know what you think of this approach. This gives at least a baseline of tests for each of the logpdf implementations (and writing these exposed a bug I had in my beta log-likelihood!)

I'll add docs to each of the distribution namespaces before we merge.

@sritchie sritchie requested a review from zane August 30, 2023 19:16
@@ -47,8 +47,8 @@
([] (uniform-distribution 0.0 1.0))
([lo hi] (->Uniform (rng) lo hi)))

(defn gaussian-distribution
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed this to match the other conventions / Gen.jl's convention.

@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Merging #35 (fe597aa) into main (e04ca19) will increase coverage by 4.01%.
Report is 3 commits behind head on main.
The diff coverage is 80.00%.

❗ Current head fe597aa differs from pull request most recent head 70df6e0. Consider uploading reports for the commit 70df6e0 to get more accurate results

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
+ Coverage   62.50%   66.51%   +4.01%     
==========================================
  Files          14       16       +2     
  Lines         616      666      +50     
  Branches       21       21              
==========================================
+ Hits          385      443      +58     
+ Misses        210      202       -8     
  Partials       21       21              
Files Coverage Δ
src/gen/distribution/math/log_likelihood.cljc 96.87% <100.00%> (ø)
src/gen/distribution/java_util.clj 79.31% <75.00%> (+31.03%) ⬆️

... and 7 files with indirect coverage changes

@@ -85,7 +85,7 @@
{:pre [(pos? alpha) (pos? beta)]}
(if (< 0 v 1)
(- (+ (* (- alpha 1) (Math/log v))
(* (- beta alpha) (Math/log (- 1 v))))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops!


The implementation follows the algorithm described on the Cauchy
distribution's [Wikipedia
page](https://en.wikipedia.org/wiki/Cauchy_distribution#Probability_density_function_(PDF))."
[scale location v]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changing these to match Gen.jl's arg order.

@zane
Copy link
Collaborator

zane commented Aug 31, 2023

@sritchie Tagging @Schaechtle in on this one!

Copy link
Collaborator

@Schaechtle Schaechtle left a comment

Choose a reason for hiding this comment

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

this looks mostly good: I left minor inline comments. However, this line indicates an error due to name clashing of gen.trace and gen/trace. If this is fixed by a different PR, feel free to update/ignore

(checking "Delta properties"
[center (gen-double -100 100)
v (gen-double -100 100)]
(if (= center v)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am confused: Unless I am misreading what gen/double* does, this will never hold. Or is there some shared random seeding that I am missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is actually calling

(defn gen-double
  "Returns a generator that produces numerical doubles between `min` and
  `max` (inclusive)."
  [min max]
  (gen/double*
   {:min min :max max :infinite? false :NaN? false}))

and with shrinking, center and v will both match probably at least once on each test run (as the generator tries the min, max, 0, etc)

But let me know if I am missing something!

(is (= -1.8862943611198906 (dist/logpdf (->laplace 0 2) 1)))
(is (= 4.214608098422191 (dist/logpdf (->laplace 0 0.001) 0.002)))))

(defn gaussian-tests [->gaussian]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this should be renamed to normal, too.

@sritchie
Copy link
Collaborator Author

@Schaechtle yep, that was a problem for cljs, fixed in #41 . Thank you!

@sritchie sritchie force-pushed the sritchie/dist_tests branch 2 times, most recently from d078f60 to fe597aa Compare September 29, 2023 18:09
@sritchie sritchie merged commit 7636f80 into main Sep 29, 2023
2 of 4 checks passed
@sritchie sritchie deleted the sritchie/dist_tests branch September 29, 2023 18:15
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.

3 participants