Add CI #19
Add CI #19
Conversation
|
@aloctavodia you can see the CI running on my fork juanitorduz#1 The tests |
|
Actually, the changes proposed ensure the test run and they are all green 🟢 🙌 juanitorduz@89581c0 |
| pt.floor((n + 1) * ((alpha - 1) / (alpha + beta - 2))), | ||
| 0, | ||
| n, | ||
| # The standard formula only applies when alpha > 1 and beta > 1 |
There was a problem hiding this comment.
In preliz we always return a number for mode. But here, so far, we have been using nan if the value is not unique. Like for uniform distributions. While returning a number can me more convenient it could also be misleading.
| q = pt.as_tensor_variable(q) | ||
|
|
||
| # Create array of all possible values in support | ||
| k_vals = pt.arange(lower, upper + 1) |
There was a problem hiding this comment.
If range is too wide (not sure what is that) we should use bisection.
There was a problem hiding this comment.
Btw, no need to decide this one now. We can open an issue and solve it later
There was a problem hiding this comment.
ok, this could be an alternative because the original tests were simply not running (not even locally for me)
There was a problem hiding this comment.
Here is an alternative (b5352bb)
Summary of changes:
The _should_use_bisection function now checks two conditions:
- Infinite bounds - uses
math.isfinite()on the evaluated values - Range too wide - if
(upper - lower) > 10_000, uses bisection to avoid creating huge arrays
This means:
- BetaBinomial with n=100 → range is 101, uses direct search (fast ✓)
- BetaBinomial with n=100,000 → range is 100,001, uses bisection (avoids memory issues ✓)
- Poisson (unbounded) → uses bisection ✓
The threshold of 10,000 is a reasonable default - large enough for most practical bounded discrete distributions, but small enough to avoid performance/memory issues.
distributions/optimization.py
Outdated
| try: | ||
| lower_t = pt.as_tensor_variable(lower) | ||
| upper_t = pt.as_tensor_variable(upper) | ||
| lower_val = float(lower_t.eval()) |
There was a problem hiding this comment.
We should keep everything symbolic.
distributions/optimization.py
Outdated
| Evaluates bounds at graph-build time for constants. | ||
| """ | ||
| try: | ||
| lower_t = pt.as_tensor_variable(lower) |
There was a problem hiding this comment.
I think we can safely assume everything is a tensor here, and then other libraries than depend on this one could make other assumptions
There was a problem hiding this comment.
So far I have c78a16c . If I do pure symbolic the test keep being stuck ¯\(ツ)/¯
2 participants
Add CI and some cleaning :)