Fix `axis` handling of `random`method in GRW #3985

Rish001 · 2020-06-30T08:48:38Z

This PR made a change in the _random() method of GaussianRandomWalk class of pymc3/distributions/timeseries.py so as to generate a matrix where individual rows are instances of Guassian Random walk starting from 0.

As discussed in #3962 there is an issue in .rvs(size) method in the scipy library when size is a tuple consisting of two items like (500,10). Due to this issue, I had to construct a loop that calls the said method for 1d array and fill up the aforementioned matrix.

AlexAndorra

Thanks a lot @Rish001, this is a good start! I left some comments and questions below. I think the failing tests are giving some good pointers as to why this implementation could fail with more than two dimensions. Hope this makes sense 🖖

pymc3/distributions/distribution.py

pymc3/sampling.py

pymc3/distributions/timeseries.py

michaelosthege

I hope my comments are at least remotely helpful :)

pymc3/distributions/timeseries.py

pymc3/sampling.py

AlexAndorra

Thanks @Rish001 and well done on making this work 👏
There are still some changes to implement, but it'll be quick as it's mostly formatting. Also, could you replicate @michaelosthege's plot from the related issue and post it here, so we can be sure the new implementation fixes things?
And thanks for opening an issue on Scipy! Looks like it'll make our lives easier when it's fixed 😅

pymc3/distributions/distribution.py

AlexAndorra · 2020-07-04T11:23:17Z

pymc3/distributions/timeseries.py

+            list_of_size = list(size)
+            size_inner_matrix = list_of_size[1:]
+            size_inner_matrix_tuple = tuple(size_inner_matrix)
+            print(size_inner_matrix_tuple)


Please remove the print statement

pymc3/sampling.py

AlexAndorra · 2020-07-04T11:26:22Z

pymc3/distributions/timeseries.py

@@ -309,8 +311,30 @@ def _random(self, sigma, mu, size, sample_shape):
        else:
            axis = 0
        rv = stats.norm(mu, sigma)
-        data = rv.rvs(size).cumsum(axis=axis)
-        data = data - data[0]  # TODO: this should be a draw from `init`, if available
+        if size is None:


Can you add a small comment above this line, explaining why we have to do all this shape-handling now and writing down the link to the issue you opened on scipy?

pymc3/distributions/timeseries.py

Co-authored-by: Alexandre ANDORRA <[email protected]>

AlexAndorra · 2020-07-04T15:43:39Z

@Rish001, did you see the Scipy guys' answer to your issue? It seems like there is no bug in .rvs but our random method uses the wrong axis to compute the cumsum. So, modifying your PR to deal with axis instead of size should make the code both simpler and more efficient.

Rish001 · 2020-07-04T16:59:45Z

@AlexAndorra I saw that after pushing the formatted code. Can you please explain me the purpose of this if statement? What kind of case is it designed to handle?


def _random(self, sigma, mu, size, sample_shape):
        """Implement a Gaussian random walk as a cumulative sum of normals."""
        if size[len(sample_shape)] == sample_shape:
            axis = len(sample_shape)
        else:
            axis = 0

Rish001 · 2020-07-04T18:25:54Z

@AlexAndorra I have modified the code to handle the axis. PFB the reproduced plot.

lucianopaz

Sorry to be able to review this so late along the line. There are some major shape related problems with this _random method.

You are assuming that the last axis of a GRW distributed random variable is the time axis, but this is the opposite of what the distribution's logp does. Furthermore, your _random method is losing the information of what we call the sample_shape (the size that is passed to random), but you will need to know it to be able to decide theaxis along which you will have to do the cumsum. I've left a comment with an example situation that will break with the current code. Sorry if my message sounds discouraging, but these shape problems are really nasty and involve many many cases to be properly fixed.

pymc3/distributions/timeseries.py

Rish001 · 2020-07-12T13:38:07Z

As discussed with @lucianopaz, I have raised a new issue here.
@AlexAndorra Are there any change to made to the code formatting or otherwise for the PR to be accepted?

lucianopaz · 2020-07-12T14:13:47Z

Thanks @Rish001, the last thing you need to do is to add a line to the ReleaseNotes.md

Rish001 · 2020-07-13T14:43:44Z

Thanks @Rish001, the last thing you need to do is to add a line to the ReleaseNotes.md

Is there any format for that? And what are the key things that I have to write?

michaelosthege · 2020-07-13T17:30:21Z

Thanks @Rish001, the last thing you need to do is to add a line to the ReleaseNotes.md

Is there any format for that? And what are the key things that I have to write?

You can sort of copy from the other lines. This one would go into the "Maintenance" section. Just describe the fix in a few words and link to this PR.

codecov · 2020-07-14T06:13:48Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f93b5e7). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #3985   +/-   ##
=========================================
  Coverage          ?   86.78%           
=========================================
  Files             ?       88           
  Lines             ?    14139           
  Branches          ?        0           
=========================================
  Hits              ?    12270           
  Misses            ?     1869           
  Partials          ?        0

Impacted Files	Coverage Δ
pymc3/distributions/distribution.py	`94.47% <ø> (ø)`
pymc3/distributions/timeseries.py	`71.93% <100.00%> (ø)`
pymc3/gp/gp.py	`92.82% <0.00%> (ø)`
pymc3/tuning/__init__.py	`100.00% <0.00%> (ø)`
pymc3/glm/families.py	`95.45% <0.00%> (ø)`
pymc3/step_methods/elliptical_slice.py	`95.12% <0.00%> (ø)`
pymc3/variational/approximations.py	`80.25% <0.00%> (ø)`
pymc3/step_methods/step_sizes.py	`100.00% <0.00%> (ø)`
pymc3/step_methods/gibbs.py	`40.00% <0.00%> (ø)`
pymc3/vartypes.py	`100.00% <0.00%> (ø)`
... and 80 more

michaelosthege · 2020-07-14T08:05:29Z

@Rish001 there seems to be a misunderstanding:
The RELEASE-NOTES.md are located in the root level of the repository. Please make a 1-line mention of the changes there.

docs/release-notes/pymc3-3.0.md

lucianopaz

Thanks @Rish001! It looks good to merge now.

AlexAndorra · 2020-07-17T08:47:00Z

Thanks @ lot @Rish001 ! (just for future reference, I'd advise giving more informative names to your PR titles and git branches).
@michaelosthege, can you approve the changes and merge please?

AlexAndorra

Ooh, actually there are some issues here, that we need to address before merging:

Typos in release notes.
Empty line changes in release notes and distribution.py.
docs/release-notes/pymc3-3.0.md shouldn't exist.
The changes in timeseries.py should be commented to be easier to understand.

I don't have time to check out locally and do the changes myself, at least for the coming week. If @michaelosthege has time before that, feel free to do so.

twiecki · 2020-07-23T21:42:10Z

@Rish001 Do you have time to do those last couple of syntax fixes?

… fixed the line changes in distributions.py

Rish001 · 2020-07-24T14:16:30Z

Ooh, actually there are some issues here, that we need to address before merging:

Typos in release notes.

Empty line changes in release notes and distribution.py.

docs/release-notes/pymc3-3.0.md shouldn't exist.

The changes in timeseries.py should be commented to be easier to understand.

I don't have time to check out locally and do the changes myself, at least for the coming week. If @michaelosthege has time before that, feel free to do so.

docs/release-notes/pymc3-3.0.md I have removed lines that I had mistakenly written there. I don't know what else I need to do.

twiecki · 2020-07-24T14:33:00Z

RELEASE-NOTES.md

@@ -9,6 +9,7 @@
 - Pass the `tune` argument from `sample` when using `advi+adapt_diag_grad` (see issue [#3965](https://github.com/pymc-devs/pymc3/issues/3965), fixed by [#3979](https://github.com/pymc-devs/pymc3/pull/3979)).
 - Add simple test case for new coords and dims feature in `pm.Model` (see [#3977](https://github.com/pymc-devs/pymc3/pull/3977)). 
 - Require ArviZ >= 0.9.0 (see [#3977](https://github.com/pymc-devs/pymc3/pull/3977)).
+- Temporarily fixed issue [#3962](https://github.com/pymc-devs/pymc3/issues/3962) by making change in the `_random()` method of `GaussianRandomWalk` class, refer to PR [#3985].Further testing revealed a new issue which is being tracked [#4010](https://github.com/pymc-devs/pymc3/issues/4010) 


I think that particular issue of sampling is fixed for good, no? It just revealed another problem.

No, When some edge cases were tested it turned out that a deeper issue exists in the function logp. This PR is only a temporary fix which would be resolved permanently with the issue #4010 fixed

The way I understand, there are two related but separate issues: 1D sampling from the GRW did not work (#3962), and the logp for >1D GRWs is wrong (#4010). This PR fixes the first completely but doesn't address the second.

Yeah that's my understanding too. I think the notion of temporary fix should be removed from the release notes

Ok then I shall make the corresponding changes in Release notes. However, I am keeping the referene to 4010

pymc3/distributions/distribution.py

RELEASE-NOTES.md

twiecki · 2020-07-24T16:26:10Z

Thanks @Rish001 and congrats on your first PR!

3962 issue fixed

fbb817a

AlexAndorra requested changes Jun 30, 2020

View reviewed changes

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

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

pymc3/sampling.py Outdated Show resolved Hide resolved

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

AlexAndorra changed the title ~~#3962 issue fixed~~ Fix randommethod of GRW because of .rvs(size) bug in Scipy Jul 1, 2020

AlexAndorra changed the title ~~Fix randommethod of GRW because of .rvs(size) bug in Scipy~~ Fix randommethod of GRW due to .rvs(size) bug in Scipy Jul 1, 2020

AlexAndorra added the bug label Jul 1, 2020

AlexAndorra linked an issue Jul 1, 2020 that may be closed by this pull request

GaussianRandomWalk prior predictive is broken #3962

Closed

AlexAndorra requested a review from michaelosthege July 1, 2020 09:00

michaelosthege requested changes Jul 1, 2020

View reviewed changes

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

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

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

pymc3/sampling.py Outdated Show resolved Hide resolved

Scaled ._random() to more than 2 dim

fbcf9e5

Rish001 force-pushed the my-feature branch from c0774de to fbcf9e5 Compare July 2, 2020 04:22

Fixed the remaining failed testcase

1218f26

AlexAndorra requested changes Jul 4, 2020

View reviewed changes

Rish001 and others added 4 commits July 4, 2020 18:33

Update pymc3/distributions/distribution.py

f72626c

Co-authored-by: Alexandre ANDORRA <[email protected]>

Update pymc3/distributions/distribution.py

ed6be58

Co-authored-by: Alexandre ANDORRA <[email protected]>

Update pymc3/distributions/timeseries.py

8317554

Co-authored-by: Alexandre ANDORRA <[email protected]>

Formatting issues fixed

5aaa0e1

handled value of axis to fix the bug

89ff040

lucianopaz requested changes Jul 4, 2020

View reviewed changes

pymc3/distributions/timeseries.py Show resolved Hide resolved

AlexAndorra changed the title ~~Fix randommethod of GRW due to .rvs(size) bug in Scipy~~ Fix axis handling of randommethod in GRW Jul 5, 2020

Rish001 mentioned this pull request Jul 12, 2020

GuassianRandomWalk fails for 2D shape #4010

Closed

Documented the change in Release-Notes

c8dfd3d

Documented RELEASENOTES

d3780a9

lucianopaz requested changes Jul 14, 2020

View reviewed changes

docs/release-notes/pymc3-3.0.md Outdated Show resolved Hide resolved

removed text from the docs/release-notes.md/pymc3-3.0.md

ef64f79

lucianopaz approved these changes Jul 15, 2020

View reviewed changes

AlexAndorra approved these changes Jul 17, 2020

View reviewed changes

AlexAndorra requested a review from michaelosthege July 17, 2020 08:45

AlexAndorra requested changes Jul 17, 2020

View reviewed changes

Rish001 added 2 commits July 24, 2020 19:17

Fix typo in release notes and line changes

a660f32

Fixed the typos in Release notes. Added comments in timeseries.py and…

710692a

… fixed the line changes in distributions.py

twiecki reviewed Jul 24, 2020

View reviewed changes

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

Removed temporary from release notes. fixed the line changes

531091f

twiecki reviewed Jul 24, 2020

View reviewed changes

RELEASE-NOTES.md Outdated Show resolved Hide resolved

Update RELEASE-NOTES.md

8e2bb93

twiecki merged commit 6d0ff91 into pymc-devs:master Jul 24, 2020

kyleabeauchamp added this to the 3.9.3 milestone Jul 28, 2020

Sayam753 mentioned this pull request Jan 2, 2021

Add .random method to MvGaussianRandomWalk (Issue #4337) #4388

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `axis` handling of `random`method in GRW #3985

Fix `axis` handling of `random`method in GRW #3985

Rish001 commented Jun 30, 2020

AlexAndorra left a comment

michaelosthege left a comment

AlexAndorra left a comment

AlexAndorra Jul 4, 2020

AlexAndorra Jul 4, 2020

AlexAndorra commented Jul 4, 2020

Rish001 commented Jul 4, 2020

Rish001 commented Jul 4, 2020

lucianopaz left a comment

Rish001 commented Jul 12, 2020

lucianopaz commented Jul 12, 2020

Rish001 commented Jul 13, 2020

michaelosthege commented Jul 13, 2020

codecov bot commented Jul 14, 2020 •

edited

Loading

michaelosthege commented Jul 14, 2020

lucianopaz left a comment

AlexAndorra commented Jul 17, 2020

AlexAndorra left a comment

twiecki commented Jul 23, 2020

Rish001 commented Jul 24, 2020

twiecki Jul 24, 2020

Rish001 Jul 24, 2020

twiecki Jul 24, 2020

AlexAndorra Jul 24, 2020

Rish001 Jul 24, 2020

twiecki commented Jul 24, 2020

Fix axis handling of randommethod in GRW #3985

Fix axis handling of randommethod in GRW #3985

Conversation

Rish001 commented Jun 30, 2020

AlexAndorra left a comment

Choose a reason for hiding this comment

michaelosthege left a comment

Choose a reason for hiding this comment

AlexAndorra left a comment

Choose a reason for hiding this comment

AlexAndorra Jul 4, 2020

Choose a reason for hiding this comment

AlexAndorra Jul 4, 2020

Choose a reason for hiding this comment

AlexAndorra commented Jul 4, 2020

Rish001 commented Jul 4, 2020

Rish001 commented Jul 4, 2020

lucianopaz left a comment

Choose a reason for hiding this comment

Rish001 commented Jul 12, 2020

lucianopaz commented Jul 12, 2020

Rish001 commented Jul 13, 2020

michaelosthege commented Jul 13, 2020

codecov bot commented Jul 14, 2020 • edited Loading

Codecov Report

michaelosthege commented Jul 14, 2020

lucianopaz left a comment

Choose a reason for hiding this comment

AlexAndorra commented Jul 17, 2020

AlexAndorra left a comment

Choose a reason for hiding this comment

twiecki commented Jul 23, 2020

Rish001 commented Jul 24, 2020

twiecki Jul 24, 2020

Choose a reason for hiding this comment

Rish001 Jul 24, 2020

Choose a reason for hiding this comment

twiecki Jul 24, 2020

Choose a reason for hiding this comment

AlexAndorra Jul 24, 2020

Choose a reason for hiding this comment

Rish001 Jul 24, 2020

Choose a reason for hiding this comment

twiecki commented Jul 24, 2020

Fix `axis` handling of `random`method in GRW #3985

Fix `axis` handling of `random`method in GRW #3985

codecov bot commented Jul 14, 2020 •

edited

Loading