Skip to content

Scipy generator methods#113

Merged
phil-brad merged 9 commits intomasterfrom
scipy-generators
Mar 26, 2020
Merged

Scipy generator methods#113
phil-brad merged 9 commits intomasterfrom
scipy-generators

Conversation

@phil-brad
Copy link
Member

Allows use of any class from scipy.stats as a method for a generator by prefixing the method with scipy.. For example:

{
  "generators": {
    "Gen1": {
      "method": "scipy.norm",
      "loc": 12.0,
      "scale": 2.5,
      "random_state": 14
    }
  }
}

The random state creates a numpy.random.RandomState that persists through the life of the generator allowing repeatable generation of values.

Scipy (and numpy) has not been added as an install requirement but the user is thrown an error if it is not installed when using a scipy generator method.

Closes #112

@phil-brad phil-brad requested a review from mtinning March 24, 2020 01:37
Copy link
Member

@mtinning mtinning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of suggestions - you might find the testing problem disappears

raise ImportError("The scipy module is not installed and therefore the 'scipy.{}'"
" generator cannot be created".format(distribution)) from ex
if not hasattr(scipy_stats_module, distribution):
raise KeyError("'{}' distribution not found in scipy.stats module")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a .format

:param kwargs: Arguments to creation of statistical function
"""
try:
scipy_stats_module = import_module('scipy.stats')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution advised when using importlib functions directly. I would rewrite this to:

try:
    import scipy.stats
except ImportError as ex:
    ....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it as suggested, although that did lead to a strange AttributeError which had to be fixed by importing in conftest.py.

However, the build is still failing because scipy is not in the environment

Copy link
Member

@mtinning mtinning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@phil-brad phil-brad merged commit 488a03c into master Mar 26, 2020
@phil-brad phil-brad deleted the scipy-generators branch March 26, 2020 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add scipy distributions as generators

2 participants