Refactor Multivariate RandomWalk distributions #6131
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ricardoV94
force-pushed
the
refactor_mv_dists
branch
from
5cfd8ff
to
c0e6776
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6131 +/- ##
===========================================
+ Coverage 81.62% 93.58% +11.96%
===========================================
Files 101 101
Lines 22093 22151 +58
===========================================
+ Hits 18034 20731 +2697
+ Misses 4059 1420 -2639
|
ricardoV94
force-pushed
the
refactor_mv_dists
branch
from
c0e6776
to
f1ab46b
Compare
ricardoV94
force-pushed
the
refactor_mv_dists
branch
2 times, most recently
from
5d52bd2
to
bf2cdcc
Compare
junpenglao
reviewed
Oct 9, 2022
ricardoV94
force-pushed
the
refactor_mv_dists
branch
from
bf2cdcc
to
dd0de56
Compare
ricardoV94
changed the title
ricardoV94
force-pushed
the
refactor_mv_dists
branch
from
dd0de56
to
06f76c6
Compare
junpenglao
approved these changes
Oct 9, 2022
There was a problem hiding this comment.
Great PR @ricardoV94, as always!
It seems like get_dists does a lot of the hard work for the dist method, but why couldn't it be a part of RandomWalk? I am guessing that the answer is inherent to metaclasses but I just can't pinpoint it right now.
| ): | ||
| raise TypeError("init_dist must be a univariate distribution variable") | ||
| raise TypeError("init_dist must be a distribution variable") | ||
| check_dist_not_registered(init_dist) | ||
|
|
||
| if not ( |
There was a problem hiding this comment.
Can this check be a helper function? I feel like we often check these conditions
There was a problem hiding this comment.
Yeah I always think about it. It's on the edge of being so short that it doesn't make complete sense. Have to think about it
| @classmethod | ||
| @abc.abstractmethod | ||
| def get_dists(cls, *args, **kwargs): | ||
| pass |
There was a problem hiding this comment.
raise NotImplementedError here maybe?
There was a problem hiding this comment.
abc.abstractmethod does that ;)
ricardoV94
force-pushed
the
refactor_mv_dists
branch
3 times, most recently
from
2787c57
to
e3a919c
Compare
* Do not call `at.stack` until the end * Allow to pass single shape_offset
Also create abstract class for predefined RandomWalks
ricardoV94
force-pushed
the
refactor_mv_dists
branch
from
e3a919c
to
b134701
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #4642
Checklist
Major / Breaking Changes
Bugfixes / New features
RandomWalkto multivariate casesDocs / Maintenance
MvGaussianRandomWalkandMvStudentTRandomWalkfor V4