Restrict domain on alpha in the CAR distribution #6801

daniel-saunders-phil · 2023-06-28T17:37:58Z

Hi, I think I need some help writing the checks correctly. The intended change is supposed to be fairly trivial. the alpha parameter should be restricted to be (-1,1). I tried making the changes that I thought would enforce them but they are not working. The make_node() check is not triggering when I create the random variable with .dist or when I use pm.draw(). The lopg() check is triggering all the time, even when alpha is in the acceptable range.

Maybe something we can discuss @fonnesbeck @bwengals today?

📚 Documentation preview 📚: https://pymc--6801.org.readthedocs.build/en/6801/

…meter checks.

ricardoV94 · 2023-06-28T17:41:47Z

pymc/distributions/multivariate.py

        alpha = pt.as_tensor_variable(floatX(alpha))
+        msg = "the domain of alpha is: -1 < alpha < 1."
+        alpha = Assert(msg)(alpha, pt.lt(alpha, 1) and pt.gt(alpha, -1))


You shouldn't put this assert here. Instead raise an error in the rng_fn if the alpha is actually problematic for random draws.

Otherwise Asserts will complicate the graph needlessly.

Anyway the reason you don't see the exception is probably that you need to use pt.and_ instead of python and.

Yup the pt.and_ was what I was missing. So instead of using assert in the rng function, I just raise a ValueError? Is that right?

if alpha >= 1 or alpha <= -1: raise ValueError("the domain of alpha is: -1 < alpha < 1")

Yes, but alpha could be a vector so you need a np.all(...)

gotcha - okay sent in a new commit with these changes.

ricardoV94 · 2023-06-28T17:44:08Z

The logp() check is triggering all the time, even when alpha is in the acceptable range.

What do you mean?

daniel-saunders-phil · 2023-06-28T17:51:08Z

The logp() check is triggering all the time, even when alpha is in the acceptable range.

What do you mean?

Here the alpha is fine:

RV = CAR.dist(W=np.array([[1,0],[0,1]]),alpha=.95,mu=[0,0],tau=1)
pm.logp(RV,np.array([1,1.5]))

but it returns Check{-1 < alpha < 1 and tau > 0}.0 instead of the logp value.

codecov · 2023-06-28T17:51:40Z

Codecov Report

Merging #6801 (26859b1) into main (7b08fc1) will decrease coverage by 6.33%.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6801      +/-   ##
==========================================
- Coverage   91.92%   85.59%   -6.33%     
==========================================
  Files          95       95              
  Lines       16197    16263      +66     
==========================================
- Hits        14889    13921     -968     
- Misses       1308     2342    +1034

Impacted Files	Coverage Δ
pymc/distributions/multivariate.py	`57.72% <0.00%> (-34.48%)`	⬇️

... and 19 files with indirect coverage changes

ricardoV94 · 2023-06-28T18:05:08Z

logp returns a symbolic graph. You need to actually compile it in a pytensor.function and call it (or for debugging call .eval) to get a number our of it.

During compilation the check will be removed if it's not needed (i.e. alpha is constant and in the required range).

ricardoV94 · 2023-06-28T18:07:59Z

This may be helpful to understand: https://www.pymc.io/projects/docs/en/stable/learn/core_notebooks/pymc_pytensor.html#enough-with-random-variables-i-want-to-see-some-log-probabilities

daniel-saunders-phil · 2023-06-28T18:12:18Z

logp returns a symbolic graph. You need to actually compile it in a pytensor.function and call it (or for debugging call .eval) to get a number our of it.

During compilation the check will be removed if it's not needed (i.e. alpha is constant and in the required range).

Oh right of course! yeah the logp is behaving properly too.

daniel-saunders-phil · 2023-07-09T01:51:01Z

I'm happy with the PR at this point - just having trouble with codecov. It looks like coverage increases on the file I changed but I'm getting a lot of indirect coverage issues that are causing it to fail. No clue what I need to fix. Does anyone know why these might be coming up?

ricardoV94 · 2023-07-09T06:02:50Z

pymc/distributions/multivariate.py

@@ -2077,6 +2078,9 @@ def rng_fn(cls, rng: np.random.RandomState, mu, W, alpha, tau, size):
        Journal of the Royal Statistical Society Series B, Royal Statistical Society,
        vol. 63(2), pages 325-338. DOI: 10.1111/1467-9868.00288
        """
+        if np.all(alpha >= 1) or np.all(alpha <= -1):


I assume you mean this?

Suggested change

if np.all(alpha >= 1) or np.all(alpha <= -1):

if np.any(alpha >= 1) or np.any(alpha <= -1):

ricardoV94 · 2023-07-09T06:04:03Z

tests/distributions/test_multivariate.py

+    car_dist = pm.CAR.dist(W=W, alpha=alpha, mu=mu, tau=tau)
+
+    with pytest.raises(ValueError, match="the domain of alpha is: -1 < alpha < 1"):
+        pm.draw(car_dist)


Maybe worth to test the logp as well?

Yeah I think so too. Is it okay if I write it within the same test or do we need two separate ones? I went with putting them in the same test with the latest commit but happy to reorganize it.

I think either is OK, it's clear what you're testing either way

ricardoV94 · 2023-07-09T06:04:40Z

The codecov here on GitHub is a bit flaky, don't worry about it

ricardoV94 · 2023-07-09T06:05:07Z

tests/distributions/test_multivariate.py

@@ -835,6 +835,23 @@ def test_car_matrix_check(sparse):
            car_dist = pm.CAR.dist(mu, W, alpha, tau)


+@pytest.mark.parametrize("alpha", [1, -1])
+def test_car_alpha(alpha):


Suggested change

def test_car_alpha(alpha):

def test_car_alpha_bounds(alpha):

bwengals

This looks good to me! It looks like codecov is comparing your branch to 7b08fc1, which was from a couple weeks ago, not current main, which is causing those indirect coverage changes. Am I correct in guessing that this is why it's a non-issue then for merging @ricardoV94?

ricardoV94 · 2023-07-12T05:06:34Z

This looks good to me! It looks like codecov is comparing your branch to 7b08fc1, which was from a couple weeks ago, not current main, which is causing those indirect coverage changes. Am I correct in guessing that this is why it's a non-issue then for merging @ricardoV94?

I never investigated why codecov bot is so flaky so it's nice to have an hypothesis. Many times I had to ignore because it was clearly wrong (as is here)

Restricted alpha to be greater than -1 and less than 1. Adjusted para…

1f00345

…meter checks.

ricardoV94 reviewed Jun 28, 2023

View reviewed changes

daniel-saunders-phil changed the title ~~Restricted alpha to be greater than -1 and less than 1. Adjusted para…~~ Restrict domain on alpha in the CAR distribution Jun 28, 2023

daniel-saunders-phil added 2 commits June 28, 2023 12:38

Ricardo's suggestions

835f533

add tests for code-cov

eba76f6

daniel-saunders-phil marked this pull request as ready for review July 9, 2023 01:35

ricardoV94 reviewed Jul 9, 2023

View reviewed changes

logp test, test rename, np.all - > np.any

26859b1

bwengals self-requested a review July 12, 2023 00:44

bwengals approved these changes Jul 12, 2023

View reviewed changes

ricardoV94 mentioned this pull request Jul 12, 2023

Upload to codecov only once all_tests are run #6660

Closed

fonnesbeck merged commit 510d3b8 into pymc-devs:main Jul 26, 2023

	if np.all(alpha >= 1) or np.all(alpha <= -1):
	if np.any(alpha >= 1) or np.any(alpha <= -1):

Uh oh!

Restrict domain on alpha in the CAR distribution #6801

Restrict domain on alpha in the CAR distribution #6801

Uh oh!

Conversation

daniel-saunders-phil commented Jun 28, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardoV94 Jun 28, 2023

Choose a reason for hiding this comment

Uh oh!

daniel-saunders-phil Jun 28, 2023

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Jun 28, 2023

Choose a reason for hiding this comment

Uh oh!

daniel-saunders-phil Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricardoV94 commented Jun 28, 2023

Uh oh!

daniel-saunders-phil commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ricardoV94 commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardoV94 commented Jun 28, 2023

Uh oh!

daniel-saunders-phil commented Jun 28, 2023

Uh oh!

daniel-saunders-phil commented Jul 9, 2023

Uh oh!

ricardoV94 Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

daniel-saunders-phil Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwengals Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

ricardoV94 commented Jul 9, 2023

Uh oh!

ricardoV94 Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

bwengals left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricardoV94 commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

daniel-saunders-phil commented Jun 28, 2023 •

edited by github-actions bot

Loading

daniel-saunders-phil Jun 28, 2023 •

edited

Loading

daniel-saunders-phil commented Jun 28, 2023 •

edited

Loading

codecov bot commented Jun 28, 2023 •

edited

Loading

ricardoV94 commented Jun 28, 2023 •

edited

Loading

daniel-saunders-phil Jul 9, 2023 •

edited

Loading

bwengals left a comment •

edited

Loading

ricardoV94 commented Jul 12, 2023 •

edited

Loading