Stats: refactory of sampling methods #20494

Upabjojr · 2020-11-26T11:01:05Z

Just cleaning up the code.

Remove duplicated code,
Make the code simpler,
Use subclasses instead of strange dictionary lookups.

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

stats
- Refactory and simplification of sampling backends for random variables.

sympy-bot · 2020-11-26T11:01:14Z

✅

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

stats
- Refactory and simplification of sampling backends for random variables. (#20494 by @czgdp1807 and @Upabjojr)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.8.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

Just cleaning up the code.
- Remove duplicated code,
- Make the code simpler,
- Use subclasses instead of strange dictionary lookups.

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed


#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
- stats
  - Refactory and simplification of sampling backends for random variables.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

czgdp1807 · 2020-11-26T15:07:20Z

N.B. #18754 (comment)

Adding a new library can involve changes across multiple distribution classes across multiple files. But as in the above comment, k is also practically small i.e., many libraries don't support a lot of distributions provided by sympy.stats. Though trade off of this kind between amount of scattered code and code cleanliness will be there always. Any further thoughts on this will be helpful to decide how to proceed on the design.

codecov · 2020-11-26T15:20:32Z

Codecov Report

Merging #20494 (3b0547d) into master (524467d) will decrease coverage by 0.079%.
The diff coverage is 2.293%.

@@              Coverage Diff              @@
##            master    #20494       +/-   ##
=============================================
- Coverage   75.781%   75.702%   -0.080%     
=============================================
  Files          673       676        +3     
  Lines       174510    174529       +19     
  Branches     41202     41191       -11     
=============================================
- Hits        132247    132123      -124     
- Misses       36546     36686      +140     
- Partials      5717      5720        +3

Upabjojr · 2020-11-26T16:00:47Z

N.B. #18754 (comment)

Adding a new library can involve changes across multiple distribution classes across multiple files. But as in the above comment, k is also practically small i.e., many libraries don't support a lot of distributions provided by sympy.stats. Though trade off of this kind between amount of scattered code and code cleanliness will be there always. Any further thoughts on this will be helpful to decide how to proceed on the design.

I don't see it as a problem. Implementing a few more methods for every distribution is really OK.

The current state of external calls is a typical debugging nightmare... putting methods inside dictionaries then looking them up. Python indeed supports first-class functions, but this kind of coding should really be avoided. The viable alternative would be @functools.singledispatch, but definitely not the current code.

Upabjojr · 2020-11-26T16:01:23Z

Furthermore, the same code was copy-pasted a few times. That's also a design flaw.

Upabjojr · 2020-11-26T16:23:15Z

In general, I keep telling everybody to code as if you were coding in C++ or Java. Using Python magic may be evil.

oscarbenjamin · 2020-11-26T16:45:52Z

It's possible to avoid the magic while still organising the sampling code in classes for each external library rather than distributed as methods over all the different distribution classes.

The current code is a little messy but there's nothing wrong with using a dict to define the mapping from sympy classes to sampling functions (it should use the classes as keys rather than strings though). The fact that it's harder to do it that way in C++ or Java does not mean that it is a bad idea in Python.

A stronger consideration to me is that we should aim to concentrate code that is specific to external libraries in one place like a compatibility layer.

Upabjojr · 2020-11-26T16:54:24Z

It's possible to avoid the magic while still organising the sampling code in classes for each external library rather than distributed as methods over all the different distribution classes.

Well, singledispatching wouldn't require a class either.

The current code is a little messy but there's nothing wrong with using a dict to define the mapping from sympy classes to sampling functions (it should use the classes as keys rather than strings though).

It's unreadable, messy and hard to debug. Since Python 3.4 there's single dispatching in the standard library, so we should use that instead of .__class__.__name__ in dictionaries.

The fact that it's harder to do it that way in C++ or Java does not mean that it is a bad idea in Python.

Too much freedom in Python may lead to messy code.

A stronger consideration to me is that we should aim to concentrate code that is specific to external libraries in one place like a compatibility layer.

Well, that could be turned the other way round. We could also concentrate the code specific to a particular distribution function into that distribution. Either way or the other.

Anyway, the only alternative I see is using singledispatch. I'm definitely going to remove that dist.__class__.__name__ horror. Let me know what you think.

oscarbenjamin · 2020-11-26T17:43:07Z

I think singledispatch is reasonable. It means we can group all the code together for each external sampling library and makes it easy to add new libraries but also makes the sampling code extensible from outside if needed by users for their own classes.

The fact that it's harder to do it that way in C++ or Java does not mean that it is a bad idea in Python.

Too much freedom in Python may lead to messy code.

It might do. You have the freedom to write code in different ways so you have a bit more responsibility to design something sensible but also more flexibility to do so.

You can make a mess with ordinary methods as well if you don't take care to design properly (and document the design) e.g. #20479.

czgdp1807 · 2020-11-27T15:30:31Z

I think the approach in the diff should be good because the only drawback is that this will require adding new methods to many distributions for supporting a new library. Since, python is interpreted language, import some_library wont' be executed until that statement is reached. In fact, with the new code, all the necessary features of a distribution will remain localized instead of sampling feature present in some other file making debugging easy.

czgdp1807 · 2020-11-27T15:39:14Z

sympy/stats/crv.py

-class SampleContinuousNumpy:
-    """Returns the sample from numpy of the given distribution"""
-
-    def __new__(cls, dist, size):
-        return cls._sample_numpy(dist, size)
-
-    @classmethod
-    def _sample_numpy(cls, dist, size):
-        """Sample from NumPy."""
-
-        import numpy
-        numpy_rv_map = {
-            'BetaDistribution': lambda dist, size: numpy.random.beta(a=float(dist.alpha),
-                b=float(dist.beta), size=size),
-            'ChiSquaredDistribution': lambda dist, size: numpy.random.chisquare(
-                df=float(dist.k), size=size),
-            'ExponentialDistribution': lambda dist, size: numpy.random.exponential(
-                1/float(dist.rate), size=size),
-            'GammaDistribution': lambda dist, size: numpy.random.gamma(float(dist.k),
-                float(dist.theta), size=size),
-            'LogNormalDistribution': lambda dist, size: numpy.random.lognormal(
-                float(dist.mean), float(dist.std), size=size),
-            'NormalDistribution': lambda dist, size: numpy.random.normal(
-                float(dist.mean), float(dist.std), size=size),
-            'ParetoDistribution': lambda dist, size: (numpy.random.pareto(
-                a=float(dist.alpha), size=size) + 1) * float(dist.xm),
-            'UniformDistribution': lambda dist, size: numpy.random.uniform(
-                low=float(dist.left), high=float(dist.right), size=size)
-        }
-
-        dist_list = numpy_rv_map.keys()
-
-        if dist.__class__.__name__ not in dist_list:
-            return None
-
-        return numpy_rv_map[dist.__class__.__name__](dist, size)
-


I see. The above two comments can be ignored.

czgdp1807 · 2020-11-27T15:42:18Z

sympy/stats/rv.py

+            # See links below referring to sections beginning with "A common parametrization..."
+            # I will remove all these comments if everything is ok.
+
+            samps = self._do_sample_scipy(size)


I think that import scipy can be written just above this line. This may help in avoid writing, import scipy for every implementation of self._do_sample_scipy as scipy will be imported into the namespace before control goes for sampling using it. Possibly, you may have tried it. Can you please let me know why it didn't work?

Same goes for other libraries if it works.

sympy-bot · 2020-11-27T20:25:11Z

🟠

Hi, I am the SymPy bot (v161). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it.

This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75.

The following commits add new files:

667eb1f:
- sympy/stats/sampling/__init__.py
- sympy/stats/sampling/sample_numpy.py
- sympy/stats/sampling/sample_pymc3.py
- sympy/stats/sampling/sample_scipy.py
- sympy/stats/sampling/tests/__init__.py
639d3ab:
- sympy/stats/sampling/tests/test_sample_continuous_rv.py
- sympy/stats/sampling/tests/test_sample_discrete_rv.py
- sympy/stats/sampling/tests/test_sample_finite_rv.py

The following commits delete files:

1166d14:
- sympy/stats/sampling/compatibility.py

If these files were added/deleted on purpose, you can ignore this message.

czgdp1807 · 2020-11-28T08:50:19Z

IMHO, the previous commit(adding sampling methods to distribution classes) was better. Now, things are way more scattered and debugging would be more difficult since features of a particular distribution are not there where they should be. Using singledispatch seems unnecessary when we just could add some simple method addition to some classes.

Upabjojr · 2020-11-28T09:52:33Z

IMHO, the previous commit(adding sampling methods to distribution classes) was better. Now, things are way more scattered and debugging would be more difficult since features of a particular distribution are not there where they should be. Using singledispatch seems unnecessary when we just could add some simple method addition to some classes.

I was following the suggestion of putting all sampling calls in the same place. In the end, what this PR does is adding methods for calling external samplers to the distribution classes. The previous commit was adding methods inside the class, this last commit adds methods outside of the class using singledispatch.

Singledispatch is now in the standard library of Python, so that's a standard way to declare methods to a class externally.

Upabjojr · 2020-11-28T09:56:52Z

What is certain... I'm going to remove all strings .__class__.__name__ in stats, I don't wanna see that code anywhere.

czgdp1807 · 2020-11-28T10:30:26Z

I don't have much problem with usage of singledispatch. The greater issue I can observe here is that what could have been easily written inside the distribution class is now in a completely separate file. I would still request to switch back to the previous diff in your PR because that is way more easy to maintain and understand, since everything related to a distribution will be in the same place rather than scattered in different files. I am okay with both the approaches but the previous one was much better.

I was following the suggestion of putting all sampling calls in the same place.

But by doing that we are putting methods related to a distribution in a different place. So one way or the other things will be scattered. I am not sure if things like singledispatch is available in C++ (let me know if there is, I will try to use that stuff in my project, especially if it is in C++11), though I haven't seen someone doing it in other languages. The former approach was more standard and closer to what people usually do for encapsulating all the features of a class.

P.S. I hope switching back to previous diff just needs a minor effort i.e., git rebase -i and dropping the last commit from git history and then a force push.

czgdp1807 · 2020-11-28T10:31:53Z

I'm going to remove all strings .__class__.__name__ in stats, I don't wanna see that code anywhere.

Sure. That's cryptic too and should be avoided whenever we have an alternative to it. No problems with this thought.

oscarbenjamin · 2020-11-28T11:40:23Z

I don't mind so much what you two want to do with this.

Personally I think that the current dict approach is fine except that it should have been using the class rather than the name of the class as the keys in the dict and also the logic was a bit convoluted.

The two approaches here are both friendlier to subclasses but I don't really see why there should be any need to support subclassing these.

Upabjojr · 2020-11-28T20:33:37Z

I am not sure if things like singledispatch is available in C++ (let me know if there is, I will try to use that stuff in my project, especially if it is in C++11), though I haven't seen someone doing it in other languages.

There is function overloading by default, though that's not exactly the same as singledispatch as the dispatch is done at compile time (instead of runtime). That's the main difference between C and C++.

Technically not even class methods are dispatched (at runtime) in C++, unless you define them as virtual. (Java does it by default instead).

In SymEngine they are using the visitor pattern to implement singledipatching (it's a trick involving the proper usage of two virtual methods). So yes, there is singledispatch. SymEngine uses it exactly for the same purpose that I am using it in this PR (i.e. not defining too many methods in classes).

I would still request to switch back to the previous diff in your PR because that is way more easy to maintain and understand, since everything related to a distribution will be in the same place rather than scattered in different files. I am okay with both the approaches but the previous one was much better.

Well, no need to rush. There are advantages and disadvantages in both approaches. SymEngine uses both IIRC.

I was initially a bit more favorable to class methods, but this second way is not so bad in the end.

czgdp1807 · 2020-11-29T09:19:00Z

visitor pattern to implement singledipatching

This is something new to me. Thanks for bringing this up, it seems to be a very nice application of Dynamic Binding.

Well, no need to rush. There are advantages and disadvantages in both approaches. SymEngine uses both IIRC.

Agreed. I think the new approach has an advantage of avoiding pollution of distribution classes with multiple methods for a multiple libraries. I didn't think of this earlier. The dict based approach can be hacked easily by getting coverage with partial testing, so removing that is a good step. Overall, I am +1 with using singledispatch now. Thanks for sharing the details of SymEngine as well.

sympy/stats/crv_types.py

sympy/stats/rv.py

czgdp1807 · 2020-12-10T06:38:04Z

I am marking this stalled until the release of 1.7.1. Once we are done with fixing all the regression issues received from the end users we will re-start working on correctly designing the sampling architecture.
I personally think that sample has improved a lot and has now very simple to use APIs. When changes happen, there are always some regression issues and we will fix those issues as we receive feedback from the users.

Upabjojr · 2020-12-11T21:43:16Z

Another advantage of @singledispatch over methods: we can overload objects from sympy/core without editing SymPy's core.

For example, if an external library can handle optimized sampling of the addion or multiplication of random variables, we can add dispatchers for Add and Mul directly... so that sample(X + Y) gets redirected to the proper function.

czgdp1807 · 2020-12-12T08:14:49Z

We may continue this in a week or so. We need minor changes here like, adding seed argument to every function. I will push all those changes here.

czgdp1807 · 2020-12-22T07:41:53Z

I will bring the back changes from 1.7 to master, starting the work from tomorrow.

Upabjojr · 2020-12-22T08:07:43Z

We may continue this in a week or so. We need minor changes here like, adding seed argument to every function. I will push all those changes here.

Classes were invented to avoid that kind of edits.

class SamplingOnSomeExternalLibrary:

    def __init__(self, seed):
        self.seed =  seed

    @singledispatchmethod
    def sample(self, x):
        pass

    @sample.register
    def _(self, x: NormalDistribution):
        return some_library.sample_normal(... , seed=self.seed)

The problem is than singledispatchmethod was introduced only in Python 3.8 (single dispatch on functions OTOH was introduced in Python 3.4).

I'm definitely in favor of singledispatching, as you can define the dispatching logic for classes in SymPy's core without editing SymPy's core. For example Add, Mul could have their special sampling functions defined in the stats module.

If we are going to use classes to avoid adding parameters to every dispatched function, we still need to find a way to make @singledispatchmethod work with Python 3.6 and Python 3.7. Maybe backporting it to sympy/utilities/compatibility ?

czgdp1807 · 2021-01-07T06:33:26Z

#20658 has been merged. We can proceed with this PR according to the latest API changes (addition of optional seed argument)

Upabjojr · 2021-01-07T08:09:14Z

We can proceed with this PR according to the latest API changes (addition of optional seed argument)

Should we use a class with @singledispatchmethod for convenience?

czgdp1807 · 2021-01-08T06:27:24Z

Should we use a class with @singledispatchmethod for convenience?

The current approach in the diff looks good.

czgdp1807 · 2021-01-13T05:30:06Z

I have moved all sampling tests for CRV, FRV and DRV to stats/sampling/test in respective files. See diff here.
I see that sampling tests fail on my system? Python version is 3.6.9. Do I need something additional to make the tests work locally?
See logs below,

================================================= test process starts ==================================================
executable:         /usr/bin/python3  (3.6.9-final-0) [CPython]
architecture:       64-bit
cache:              yes
ground types:       python 
numpy:              1.19.2
random seed:        54908228
hash randomization: on (PYTHONHASHSEED=470444264)

sympy/stats/sampling/tests/test_sample_continuous_rv.py[3] EEs                                                    [FAIL]
sympy/stats/sampling/tests/test_sample_discrete_rv.py[3] EEs                                                      [FAIL]
sympy/stats/sampling/tests/test_sample_finite_rv.py[4] EEs/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py:1110: UserWarning: 
The return type of sample has been changed to return an iterator
object since version 1.7. For more information see
https://github.com/sympy/sympy/issues/19061
  warnings.warn(filldedent(message))
.                                                       [FAIL]

________________________________________________________________________________________________________________________
______________________ sympy/stats/sampling/tests/test_sample_continuous_rv.py:test_sample_numpy _______________________
Traceback (most recent call last):
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/sampling/tests/test_sample_continuous_rv.py", line 25, in test_sample_numpy
    samps = next(sample(X, size=size, library='numpy'))
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1283, in return_generator_finite
    library=library, seed=seed) # a dictionary that maps RVs to values
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/crv.py", line 473, in sample
    return {self.value: self.distribution.sample(size, library=library, seed=seed)}
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1565, in sample
    % (self, library))
NotImplementedError: Sampling for BetaDistribution(1, 1) is not currently implemented from numpy
________________________________________________________________________________________________________________________
______________________ sympy/stats/sampling/tests/test_sample_continuous_rv.py:test_sample_scipy _______________________
Traceback (most recent call last):
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/sampling/tests/test_sample_continuous_rv.py", line 58, in test_sample_scipy
    g_sample = list(sample(Gamma("G", 2, 7), size=size, numsamples=numsamples))
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1283, in return_generator_finite
    library=library, seed=seed) # a dictionary that maps RVs to values
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/crv.py", line 473, in sample
    return {self.value: self.distribution.sample(size, library=library, seed=seed)}
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1565, in sample
    % (self, library))
NotImplementedError: Sampling for GammaDistribution(2, 7) is not currently implemented from scipy
________________________________________________________________________________________________________________________
_______________________ sympy/stats/sampling/tests/test_sample_discrete_rv.py:test_sample_numpy ________________________
Traceback (most recent call last):
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/sampling/tests/test_sample_discrete_rv.py", line 20, in test_sample_numpy
    samps = next(sample(X, size=size, library='numpy'))
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1283, in return_generator_finite
    library=library, seed=seed) # a dictionary that maps RVs to values
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/drv.py", line 288, in sample
    return {self.value: self.distribution.sample(size, library=library, seed=seed)}
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1565, in sample
    % (self, library))
NotImplementedError: Sampling for GeometricDistribution(0.5) is not currently implemented from numpy
________________________________________________________________________________________________________________________
_______________________ sympy/stats/sampling/tests/test_sample_discrete_rv.py:test_sample_scipy ________________________
Traceback (most recent call last):
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/sampling/tests/test_sample_discrete_rv.py", line 50, in test_sample_scipy
    z_sample = list(sample(Zeta("G", 7), size=size, numsamples=numsamples))
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1283, in return_generator_finite
    library=library, seed=seed) # a dictionary that maps RVs to values
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/drv.py", line 288, in sample
    return {self.value: self.distribution.sample(size, library=library, seed=seed)}
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1565, in sample
    % (self, library))
NotImplementedError: Sampling for ZetaDistribution(7) is not currently implemented from scipy
________________________________________________________________________________________________________________________
________________________ sympy/stats/sampling/tests/test_sample_finite_rv.py:test_sample_numpy _________________________
Traceback (most recent call last):
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/sampling/tests/test_sample_finite_rv.py", line 19, in test_sample_numpy
    samps = next(sample(X, size=size, library='numpy'))
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1283, in return_generator_finite
    library=library, seed=seed) # a dictionary that maps RVs to values
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/frv.py", line 344, in sample
    return {self.value: self.distribution.sample(size, library, seed)}
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1565, in sample
    % (self, library))
NotImplementedError: Sampling for BinomialDistribution(5, 0.4, 1, 0) is not currently implemented from numpy
________________________________________________________________________________________________________________________
________________________ sympy/stats/sampling/tests/test_sample_finite_rv.py:test_sample_scipy _________________________
Traceback (most recent call last):
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/sampling/tests/test_sample_finite_rv.py", line 47, in test_sample_scipy
    h_sample = list(sample(Hypergeometric("H", 1, 1, 1), size=size, numsamples=numsamples))
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1283, in return_generator_finite
    library=library, seed=seed) # a dictionary that maps RVs to values
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/frv.py", line 344, in sample
    return {self.value: self.distribution.sample(size, library, seed)}
  File "/home/czgdp1807ssd/sympy_project/sympy/sympy/stats/rv.py", line 1565, in sample
    % (self, library))
NotImplementedError: Sampling for HypergeometricDistribution(1, 1, 1) is not currently implemented from scipy

========================== tests finished: 1 passed, 3 skipped, 6 exceptions, in 0.42 seconds ==========================
DO *NOT* COMMIT!

Upabjojr · 2021-01-13T07:31:05Z

NotImplementedError: Sampling for HypergeometricDistribution(1, 1, 1) is not currently implemented from scipy

What version of scipy are you using? pip freeze | grep scipy?

czgdp1807 · 2021-01-14T05:40:51Z

Python 3.6.9 (default, Jul 17 2020, 12:50:27) 
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import scipy
>>> scipy.__version__
'1.5.3'

The tests on master run perfectly with this version. Is it due to singledispatch? Is python version sufficient for it?

Upabjojr · 2021-01-14T08:37:45Z

Is it due to singledispatch? Is python version sufficient for it?

@singledispatch has been introduced in Python 3.4, that should not be the problem.

Apparently, SymPy uses the package python-scipy from Ubuntu Xenial:

sympy/.travis.yml

Line 65 in dc75152

- python-scipy

Upabjojr · 2021-01-14T08:41:14Z

Sorry, they are installed with conda:

sympy/.travis.yml

Line 224 in dc75152

travis_wait 30 conda create -q -n test-environment ${TEST_OPT_DEPENDENCY};

oscarbenjamin · 2021-01-14T09:22:09Z

Travis isn't used any more so testing is done with the github actions config in .github/workflows

czgdp1807 · 2021-01-15T06:56:00Z

sympy/stats/sampling/sample_numpy.py

+
+
+@singledispatch
+def do_sample_numpy(dist, size, rand_state):


This was called irrespective of the type of dist and hence, None was returned leading to NotImplementedError.

czgdp1807 · 2021-01-15T06:58:33Z

sympy/stats/sampling/sample_numpy.py

+
+# CRV:
+
+@do_sample_numpy.register(BetaDistribution)


do_sample_numpy wasn't registered with BetaDistribution at least in my system and therefore wasn't overridden. Similar was the case with all other distributions. Anyways, @do_sample_numpy.register(BetaDistribution) is more stable and less risky than @do_sample_numpy.register and depending on type annotations.

czgdp1807 · 2021-01-16T07:31:39Z

I think all is good with this PR. Will merge in a couple of days if no objections raised. Thanks.

Upabjojr added 2 commits November 26, 2020 01:04

Moved dictionary of sampling lambdas to methods of classes.

fbd0099

more code cleaning and refactoring

fef1e51

czgdp1807 added the stats label Nov 26, 2020

czgdp1807 reviewed Nov 27, 2020

View reviewed changes

refactory using singledispatch

667eb1f

fix imports

f4b2e1e

czgdp1807 reviewed Nov 29, 2020

View reviewed changes

sympy/stats/crv_types.py Show resolved Hide resolved

czgdp1807 reviewed Nov 29, 2020

View reviewed changes

sympy/stats/rv.py Outdated Show resolved Hide resolved

Upabjojr added 2 commits November 30, 2020 00:54

Moving sampling tests to subfolder

639d3ab

change order of modules in setup.py

3b0547d

czgdp1807 mentioned this pull request Dec 5, 2020

Added seed argument to sample for localising reproducibility to a specific distribution #20528

Merged

czgdp1807 added the Stalled label Dec 10, 2020

czgdp1807 removed the Stalled label Jan 7, 2021

Upabjojr added 3 commits January 7, 2021 09:55

merged from master

16d890d

WIP, adapted samplers to random state

1166d14

flake8 and restored missing line

0ca3021

czgdp1807 and others added 4 commits January 8, 2021 11:59

Merge branch 'master' into fix_sampling_subclasses_methods

73d4e48

fix after merging from master; fix missing seed in finite rv sampling

a3a82c9

remove duplicate in test_args

2b0094f

Merge branch 'master' into fix_sampling_subclasses_methods

64c692a

fixed do_sample_* methods

5401141

czgdp1807 reviewed Jan 15, 2021

View reviewed changes

removed unused imports

569aa79

czgdp1807 merged commit 83103ae into sympy:master Jan 18, 2021


		# CRV:

		@do_sample_numpy.register(BetaDistribution)

Stats: refactory of sampling methods #20494

Stats: refactory of sampling methods #20494

Conversation

Upabjojr commented Nov 26, 2020

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Nov 26, 2020 • edited Loading

czgdp1807 commented Nov 26, 2020

codecov bot commented Nov 26, 2020 • edited Loading

Codecov Report

Upabjojr commented Nov 26, 2020

Upabjojr commented Nov 26, 2020

Upabjojr commented Nov 26, 2020

oscarbenjamin commented Nov 26, 2020

Upabjojr commented Nov 26, 2020

oscarbenjamin commented Nov 26, 2020

czgdp1807 commented Nov 27, 2020

This comment was marked as resolved.

This comment was marked as off-topic.

czgdp1807 Nov 27, 2020

Choose a reason for hiding this comment

czgdp1807 Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

czgdp1807 Nov 27, 2020

Choose a reason for hiding this comment

sympy-bot commented Nov 27, 2020 • edited Loading

🟠

czgdp1807 commented Nov 28, 2020 • edited Loading

Upabjojr commented Nov 28, 2020

Upabjojr commented Nov 28, 2020

czgdp1807 commented Nov 28, 2020 • edited Loading

czgdp1807 commented Nov 28, 2020

oscarbenjamin commented Nov 28, 2020

Upabjojr commented Nov 28, 2020

czgdp1807 commented Nov 29, 2020

czgdp1807 commented Dec 10, 2020

Upabjojr commented Dec 11, 2020

czgdp1807 commented Dec 12, 2020

czgdp1807 commented Dec 22, 2020

Upabjojr commented Dec 22, 2020

czgdp1807 commented Jan 7, 2021

Upabjojr commented Jan 7, 2021

czgdp1807 commented Jan 8, 2021

czgdp1807 commented Jan 13, 2021 • edited Loading

Upabjojr commented Jan 13, 2021

czgdp1807 commented Jan 14, 2021

Upabjojr commented Jan 14, 2021

Upabjojr commented Jan 14, 2021

oscarbenjamin commented Jan 14, 2021

czgdp1807 Jan 15, 2021

Choose a reason for hiding this comment

czgdp1807 Jan 15, 2021

Choose a reason for hiding this comment

czgdp1807 commented Jan 16, 2021

sympy-bot commented Nov 26, 2020 •

edited

Loading

codecov bot commented Nov 26, 2020 •

edited

Loading

czgdp1807 Nov 27, 2020 •

edited

Loading

sympy-bot commented Nov 27, 2020 •

edited

Loading

czgdp1807 commented Nov 28, 2020 •

edited

Loading

czgdp1807 commented Nov 28, 2020 •

edited

Loading

czgdp1807 commented Jan 13, 2021 •

edited

Loading