Port Truncated Normal and Wald Distributions to V4 #4711

matteo-pallini · 2021-05-23T12:17:31Z

Port Truncated Normal and Wald to V4 as per #4686 guidelines

Still need to do/check the followings:

TruncatedNormal

Need to investigate why pymc3.tests.test_model.TestValueGradFunction.test_aesara_switch_broadcast_edge_cases_2 is failing
Is it fine to rewrite a new RV? according to the issue there should be one already, but I couldn't find it
Is it fine to pass transform as argument in dist ?
Is _defaultval deprecated? I haven't been able to find any use of it

Wald

Refactor as per guidelines
Investigate why pymc3.tests.test_distributions_random.TestWaldAlpha is failing.

ricardoV94 · 2021-05-23T12:21:53Z

Is it fine to rewrite a new RV? according to the issue there should be one already, but I couldn't find it

I might have confused it with the truncexpon

pymc3/distributions/continuous.py

ricardoV94 · 2021-05-23T12:38:21Z

pymc3/distributions/continuous.py

+        lower, lower_check, upper, upper_check = _truncated_normal_prepare_lower_and_upper(
+            lower, upper
+        )
+        print(lower.eval())


print statement

Was it considered to add to the pre-commit checks also a check for print statements?

@MarcoGorelli suggested a way this could be done on the slack, following your suggestion. Would either one of you be interested in implementing this?

I'm gonna be pretty busy in the next two weeks, but this script should work as a local hook:

import ast import sys class Visitor(ast.NodeVisitor): def __init__(self, file): self.file = file def visit_Call(self, node: ast.Call) -> None: if isinstance(node.func, ast.Name) and node.func.id == 'print': sys.stdout.write(f'{self.file}:{node.lineno}:{node.col_offset} found print statement\n') sys.exit(1) if __name__ == '__main__': for file in sys.argv[1:]: with open(file) as fd: content = fd.read() tree = ast.parse(content) visitor = Visitor(file) visitor.visit(tree)

@DRabbit17 if you wanted to submit this as a separate PR, I'll review it

Happy to give it a stab

ricardoV94 · 2021-05-23T14:13:17Z

Is _defaultval deprecated? I haven't been able to find any use of it

Kind of, we still need to refactor the testval/ initialization point logic for V4 as discussed in #4567

pymc3/distributions/continuous.py

ricardoV94 · 2021-05-23T20:54:05Z

Is it find to pass transform as argument in dist as per TruncatedNormal?

I am not sure what you mean. The BoundedContinuous class takes care of default transforms. It is possible for a user to specify another transform which will overwrite the default one. The TruncatedNormal.dist() initialization, on the other hand, will not do anything with transforms, they only matter for normal distributions intialized within a model.

I am not sure which of these you are referring to.

ricardoV94 · 2021-05-23T20:59:58Z

Need to investigate why pymc3.tests.test_model.TestValueGradFunction.test_aesara_switch_broadcast_edge_cases_2 is failing

That test probably needs to be slightly refactored for V4. The dlogp call might need to be tweaked. What error are you seeing?

matteo-pallini · 2021-05-23T21:58:41Z

I am not sure what you mean. The BoundedContinuous ...
Sorry, the bullet point wasn't really helpful. I wrote it as a reference for myself and didn't consider how cryptic it was. I was referring to the 2nd case you mentioned.

I thought that the transform argument if passed to dist could have reached the rv_op call in Distribution.dist through **kwargs and that the logic to handle transform had been moved to aesara. But actually that would have not worked python-wise. So, I guess that the transform logic (and argument) will be simply removed with the refactoring.

The dlogp call might need to be tweaked. What error are you seeing?
m.dlogp([mu])({"mu": 0}) is consistently 0. I need to familiarize with the internals of Model and logp/dlogp before being able to tweak the test appropriately. Thanks for letting me know that changing the test is an option worth considering

ricardoV94 · 2021-05-24T07:08:10Z

I thought that the transform argument if passed to dist could have reached the rv_op call in Distribution.dist through **kwargs and that the logic to handle transform had been moved to aesara. But actually that would have not worked python-wise. So, I guess that the transform logic (and argument) will be simply removed with the refactoring.

Only the first argument (the list of parameters) and size/shape are ever passed to the rv_op. The other kwargs are intercepted in Distribution.__new__() to be used there or forwarded to Model.register_rv()

ricardoV94 · 2021-05-24T16:12:10Z

@DRabbit17 I pushed a tiny change for the failing test. The issue was that we were passing the RandomVariable to the dlogp function instead of the logp "value" variable. It is an expected V3->V4 refactoring

ricardoV94 · 2021-06-07T11:48:58Z

Hi @DRabbit17, any progress on this PR?

We merged V4 into main so you will have to redirect the target of this PR.

Let me know if you need any help.

matteo-pallini · 2021-06-07T17:31:56Z

We merged V4 into main so you will have to redirect the target of this PR.

Thanks for the update and congrats!

Hi @DRabbit17, any progress on this PR?

Sorry for the lack of progress here. During the last 2-3 weeks work left me with little/no mental bandwidth. I should be able to pick it up again on the weekend. I would like to, but please feel free to re-assign the issue to someone else in case I am being a blocker, or someone else is keen to pick it earlier than that.

ricardoV94 · 2021-06-07T17:36:16Z

@DRabbit17 There is no rush, just wanted to check what was your status.

Also, do I understand correctly from the PR title that you intended to refactor the Wald distribution?

matteo-pallini · 2021-06-07T18:09:47Z

you intended to refactor the Wald distribution?

Yes

pymc3/distributions/continuous.py

pymc3/tests/test_distributions_random.py

pymc3/distributions/continuous.py

pymc3/tests/test_distributions.py

ricardoV94 · 2021-06-30T08:46:10Z

pymc3/tests/test_distributions.py

+        assert lower_interval.value == -1
+        assert upper_interval is None
+
+    def test_rich_context(self):


I think we can remove this one. Does not seem to test anything extra

The error trace you shared below is coming from that test, Originally I left it because I was expecting it to pass and it didn't, so I wanted to investigate why. Judging by this thread I would say that the warning is unrelated.

The error seems to be coming from pymc3.model.Model.set_initval. When running the Truncated with lower=None there is a mismatch in the number of dimensions between the rv_value_var (which is a scalar) and the initval (which is an array). The second is generated by initval_fn(), but its ndim is due to transform.forward(rv_var, value)`, I think. So, it may be possible that the interval is returning a wrong value due to the refactoring. But I haven't been able to replicate the issue with simpler tests, so I may be simply wrong. I may have written the test incorrectly, or there may be a bug. I think it's worth keeping the test at least until we cannot make it pass. For now I removed it though

I also saw that thread about not being harmful... but it's suspicious that the warning appears just after the failure, which comes from an Aesara check that ndims did not change and the warning does not appear in the other successful runs of the same test.

ricardoV94 · 2021-06-30T10:34:57Z

Seeing a weird warning in the failing jobs: https://github.com/pymc-devs/pymc3/pull/4711/checks?check_run_id=2950387511#step:7:2342

/usr/share/miniconda/envs/pymc3-dev-py37/lib/python3.7/importlib/_bootstrap.py:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 192 from C header, got 216 from PyObject

Which seems to be related to the failure just before: https://github.com/pymc-devs/pymc3/pull/4711/checks?check_run_id=2951980417#step:7:2332

          if self.ndim != data.ndim:
            raise TypeError(
>               f"Wrong number of dimensions: expected {self.ndim},"
                f" got {data.ndim} with shape {data.shape}."
            )
E           TypeError: Wrong number of dimensions: expected 0, got 1 with shape (1,).

ricardoV94 · 2021-07-02T08:16:51Z

@DRabbit17 check my last commit. I removed some tests (or tests_to_run) that felt unnecessary. Let me know if you disagree. Otherwise I think this PR is ready to merge.

I tested creating a bunch of TruncatedNormals with random sizes and lower/ upper parameters in a single model and I did not find any issues with the initval like the one we were getting in that rich_context model. So I am pretty confident that it was not an issue on our side.

matteo-pallini · 2021-07-02T15:30:06Z

Otherwise I think this PR is ready to merge

Agreed, I did a very small change. sorry for dragging the PR for so long and thanks for the support (it would have been way faster for you to simply do the whole thing yourself :-) ).

So I am pretty confident that it was not an issue on our side.

our side as this PR or PyMC?

I have been trying to replicate the test failure for pymc3/tests/test_distributions_random.py::TestNestedRandom::test_TruncatedNormal locally but I haven't been able to, can you? (the same goes for the rich_context test). Are the github tests being ran through a docker container? if so, is it possible to download it?

ricardoV94 · 2021-07-02T15:41:06Z

Agreed, I did a very small change. sorry for dragging the PR for so long and thanks for the support (it would have been way faster for you to simply do the whole thing yourself :-) ).

I disagree. The dynamic interval thing is something that we needed to figure out, and will be used in other places as well. It was really great that you dived in and started figuring it out.

So I am pretty confident that it was not an issue on our side.

our side as this PR or PyMC?

Both. I am pretty confident it was an issue with incompatible numpy / aesara binaries, that emerged on that specific environment.

I have been trying to replicate the test failure for pymc3/tests/test_distributions_random.py::TestNestedRandom::test_TruncatedNormal locally but I haven't been able to, can you? (the same goes for the rich_context test). Are the github tests being ran through a docker container? if so, is it possible to download it?

The TestNestedRandom seems like a weird caching issue, because we recently made tests marked as xfail strict (the tests fail if the test pass) and in this case it seems to still be surprised that the tests are passing. https://github.com/pymc-devs/pymc3/pull/4711/checks?check_run_id=2973646757#step:8:682

I can't find a explicit error message in the logs related to the "failing tests" (@michaelosthege any ideas?)

michaelosthege · 2021-07-02T16:31:25Z

I added the mark.xfail on test_TruncatedNormal because the test used a nonexistent API, but the distribution was not yet refactored.
This PR refactored the distribution, so now the test is passing.
👉 Just remove the xfail.

- remove upper and lower checks and check for lack of bounds in logp and intervals directly - pass default value for dist as `testval` to Distribution dist method

…rom numpy wald

michaelosthege · 2021-07-02T18:00:39Z

Rebased and removed that mark.xfail. Should go green now, but please check if my rebase didn't mess up anything :)

ricardoV94 · 2021-07-02T19:07:22Z

I added the mark.xfail on test_TruncatedNormal because the test used a nonexistent API, but the distribution was not yet refactored.
This PR refactored the distribution, so now the test is passing.
👉 Just remove the xfail.

I was convinced that I had removed it xD

michaelosthege · 2021-07-02T19:20:36Z

@ricardoV94 the rebase was tricky too. You probably ended up with another tests mark.xfail earlier.

Both test failures seem to be related to the known problem with the find_MAP #4771.
Merge away?

ricardoV94 · 2021-07-02T19:25:47Z

@ricardoV94 the rebase was tricky too. You probably ended up with another tests mark.xfail earlier.

Both test failures seem to be related to the known problem with the find_MAP #4771.
Merge away?

The MLE one will be adjusted in #4833. ~~Let me check the other one quickly~~ Yeah the second one looks another MAP issue. Does not seem related to any changes in this PR

ricardoV94 · 2021-07-02T19:32:21Z

Great work @DRabbit17 This was a fun one to crack. Looking forward to your next PR :)

ricardoV94 mentioned this pull request May 23, 2021

Port remaining distributions to v4 #4686

Closed

26 tasks

ricardoV94 reviewed May 23, 2021

View reviewed changes

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved

matteo-pallini marked this pull request as draft May 24, 2021 07:30

matteo-pallini force-pushed the refactor-wald-and-truncated-normal branch from 3678d71 to e5cc1f4 Compare June 13, 2021 14:07

matteo-pallini changed the base branch from v4 to main June 13, 2021 14:07

matteo-pallini force-pushed the refactor-wald-and-truncated-normal branch from e5cc1f4 to 7756579 Compare June 13, 2021 14:20

ricardoV94 reviewed Jun 13, 2021

View reviewed changes

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved

matteo-pallini force-pushed the refactor-wald-and-truncated-normal branch from 7756579 to fe86dcd Compare June 13, 2021 16:22

matteo-pallini force-pushed the refactor-wald-and-truncated-normal branch 2 times, most recently from 7fc3841 to 72a9798 Compare June 20, 2021 21:42

matteo-pallini commented Jun 20, 2021

View reviewed changes

pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved

ricardoV94 added this to the vNext (4.0.0) milestone Jun 22, 2021

ricardoV94 reviewed Jun 26, 2021

View reviewed changes

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Jun 26, 2021

View reviewed changes

pymc3/tests/test_distributions.py Show resolved Hide resolved

matteo-pallini force-pushed the refactor-wald-and-truncated-normal branch 2 times, most recently from b1aab03 to e892986 Compare June 26, 2021 23:38

matteo-pallini marked this pull request as ready for review June 26, 2021 23:48

matteo-pallini changed the title ~~WIP: Port Truncated Normal and Wald Distributions to V4~~ Port Truncated Normal and Wald Distributions to V4 Jun 26, 2021

ricardoV94 force-pushed the refactor-wald-and-truncated-normal branch 2 times, most recently from af6a560 to 44343e3 Compare June 30, 2021 08:44

ricardoV94 reviewed Jun 30, 2021

View reviewed changes

matteo-pallini force-pushed the refactor-wald-and-truncated-normal branch from 470f1e0 to a68aee1 Compare June 30, 2021 22:52

matteo-pallini and others added 12 commits July 2, 2021 19:16

Refactor Truncated Normal

b8c2a95

Fix failing test case

b265c93

Address review feedback

8dfa4ed

- remove upper and lower checks and check for lack of bounds in logp and intervals directly - pass default value for dist as `testval` to Distribution dist method

Refactor Wald distribution

621ffed

Add logp and transform tests for truncated normal

8c953be

Move Wald distribution to use the numpy rvs implementation

09ddeca

Simplify None bound replacement

7d80beb

Make TestBoundedContinuous more readable

acf0caa

Add arrays test for Wald, remove useless test and simplify sampling f…

e72d9d5

…rom numpy wald

Remove redundant tests

c89826e

remove unnecessary variable assignment

4449ef6

Remove xfail mark on TrucatedNormal tests

58b6158

michaelosthege force-pushed the refactor-wald-and-truncated-normal branch from 42fef02 to 58b6158 Compare July 2, 2021 17:59

ricardoV94 approved these changes Jul 2, 2021

View reviewed changes

ricardoV94 merged commit 9d90c89 into pymc-devs:main Jul 2, 2021

MarcoGorelli mentioned this pull request Jul 25, 2021

no print statements #4878

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port Truncated Normal and Wald Distributions to V4 #4711

Port Truncated Normal and Wald Distributions to V4 #4711

matteo-pallini commented May 23, 2021 •

edited

Loading

ricardoV94 commented May 23, 2021

ricardoV94 May 23, 2021

matteo-pallini May 23, 2021

ricardoV94 May 23, 2021

MarcoGorelli May 23, 2021

matteo-pallini May 23, 2021

ricardoV94 commented May 23, 2021 •

edited

Loading

ricardoV94 commented May 23, 2021

ricardoV94 commented May 23, 2021 •

edited

Loading

matteo-pallini commented May 23, 2021

ricardoV94 commented May 24, 2021

ricardoV94 commented May 24, 2021 •

edited

Loading

ricardoV94 commented Jun 7, 2021 •

edited

Loading

matteo-pallini commented Jun 7, 2021 •

edited

Loading

ricardoV94 commented Jun 7, 2021 •

edited

Loading

matteo-pallini commented Jun 7, 2021

ricardoV94 Jun 30, 2021

matteo-pallini Jun 30, 2021

ricardoV94 Jul 1, 2021

ricardoV94 commented Jun 30, 2021 •

edited

Loading

ricardoV94 commented Jul 2, 2021 •

edited

Loading

matteo-pallini commented Jul 2, 2021 •

edited

Loading

ricardoV94 commented Jul 2, 2021 •

edited

Loading

michaelosthege commented Jul 2, 2021

michaelosthege commented Jul 2, 2021

ricardoV94 commented Jul 2, 2021 •

edited

Loading

michaelosthege commented Jul 2, 2021

ricardoV94 commented Jul 2, 2021 •

edited

Loading

ricardoV94 commented Jul 2, 2021 •

edited

Loading

Port Truncated Normal and Wald Distributions to V4 #4711

Port Truncated Normal and Wald Distributions to V4 #4711

Conversation

matteo-pallini commented May 23, 2021 • edited Loading

ricardoV94 commented May 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardoV94 commented May 23, 2021 • edited Loading

ricardoV94 commented May 23, 2021

ricardoV94 commented May 23, 2021 • edited Loading

matteo-pallini commented May 23, 2021

ricardoV94 commented May 24, 2021

ricardoV94 commented May 24, 2021 • edited Loading

ricardoV94 commented Jun 7, 2021 • edited Loading

matteo-pallini commented Jun 7, 2021 • edited Loading

ricardoV94 commented Jun 7, 2021 • edited Loading

matteo-pallini commented Jun 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardoV94 commented Jun 30, 2021 • edited Loading

ricardoV94 commented Jul 2, 2021 • edited Loading

matteo-pallini commented Jul 2, 2021 • edited Loading

ricardoV94 commented Jul 2, 2021 • edited Loading

michaelosthege commented Jul 2, 2021

michaelosthege commented Jul 2, 2021

ricardoV94 commented Jul 2, 2021 • edited Loading

michaelosthege commented Jul 2, 2021

ricardoV94 commented Jul 2, 2021 • edited Loading

ricardoV94 commented Jul 2, 2021 • edited Loading

matteo-pallini commented May 23, 2021 •

edited

Loading

ricardoV94 commented May 23, 2021 •

edited

Loading

ricardoV94 commented May 23, 2021 •

edited

Loading

ricardoV94 commented May 24, 2021 •

edited

Loading

ricardoV94 commented Jun 7, 2021 •

edited

Loading

matteo-pallini commented Jun 7, 2021 •

edited

Loading

ricardoV94 commented Jun 7, 2021 •

edited

Loading

ricardoV94 commented Jun 30, 2021 •

edited

Loading

ricardoV94 commented Jul 2, 2021 •

edited

Loading

matteo-pallini commented Jul 2, 2021 •

edited

Loading

ricardoV94 commented Jul 2, 2021 •

edited

Loading

ricardoV94 commented Jul 2, 2021 •

edited

Loading

ricardoV94 commented Jul 2, 2021 •

edited

Loading

ricardoV94 commented Jul 2, 2021 •

edited

Loading