Create `gempyor.distributions` module by emprzy · Pull Request #575 · HopkinsIDD/flepiMoP

emprzy · 2025-06-20T18:32:32Z

Describe your changes.

This pull request creates a distributions module that contains classes with sampling methods for a variety of distributions, and corresponding tests/validation for these classes in a new directory, gempyor_pkg/tests/distributions/.

Does this pull request make any user interface changes? If so please describe.

No user interface changes.

What does your pull request address? Tag relevant issues.

This pull request was spun out of #570, and addresses what is requested in #573 (@MacdonaldJoshuaCaleb , see Gamma, Weibull, and Beta distributions added to distributions.py).

TimothyWillard

Let's rename the test files from test_ObjectName.py to test_object_name_class.py to match other test files. For future reference I think we only need to unit test custom field/model validators, we can trust that the folks at pydantic have throughly unit tested validations provided by their package.

flepimop/gempyor_pkg/src/gempyor/distributions.py

emprzy · 2025-06-24T18:40:23Z

@TimothyWillard , just committed with your suggestions. Let me know if you want each of the .sample() methods to have full docstrings, or if you want an examples >>> section in the ABC.sample() method.

TimothyWillard

Looks good to me. An example for DistributionABC.sample doesn't make much sense because it's an ABC so we can pass on that. Not exactly sure how the documentation renders here, but I think DistributionABC.sample's docstring applies to all subclasses here so we can also pass on that.

flepimop/gempyor_pkg/src/gempyor/distributions.py

Added a module for representing distributions that can be used throughout gempyor. Started with `DistributionABC` as and abstract base for all distributions. Implemented `FixedDistribution` and `NormalDistribution` using that ABC. Finally, exposed a field discriminated type to easily create distributions from config.

Added a representation for uniform distributions to `gempyor.distributions`.

Added a representation of a poisson distribution to `gempyor.distributions`

Added a representation for binomial distribution to `gempyor.distributions`.

Added a lognormal distribution to `gempyor.distributions`.

Added a truncated normal distribution to `gempyor.distributions`.

Added a representation for gamma distributions to `gempyor.distributions`

Added a representation for Weibull distributions to `gempyor.distributions`. Also, fixed a return type hint error.

Unit tests and validation for the `NormalDistribution` class in `gempyor_pkg/tests/distributions/`

Unit tests and validation for the `FixedDistribution` class in `gmepyor_pkg/tests/distributions/`. Also removal of an unused parameter in the `FixedDistribution` `.sample()` method.

Unit tests and validation for the `UniformDistribution` class in `gempyor_pkg/tests/distributions/`. Also linting from a previous commit I forgot to add.

Unit tests and validation for the `LognormalDistribution` class in `gempyor_pkg/tests/distributions/` + a fix for an issue that I created in `2c5fce5`

Add tests for the `TruncatedNormalDistribution` class from `gempyor.distributions`. Also, formatting fixes and bound fixes.

Unit tests and validation for `PoissonDIstribution` class in `gempyor.distributions`. Also, formatting and bounds fixes.

flepimop/gempyor_pkg/src/gempyor/distributions.py

Take 10 samples rather than 1 to reduce the risk of an identical series of samples being drawn (causing a test failure when arrays are compared).

TimothyWillard

Minor comment about not wanting the DistributionABC.rng property to be public, but otherwise looks good to me.

flepimop/gempyor_pkg/src/gempyor/distributions.py

use pydantic capabilities for default_factory Co-authored-by: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com>

pearsonca

Looking basically right. A few definite TODOs, but two items for discussion?

@emprzy @TimothyWillard what are your thoughts re 1) testing the initializer and 2) testing the output ranges?

depending how those opinions go, there's a bit of stripping out to do alongside the other fixes.

flepimop/gempyor_pkg/tests/distributions/test_beta_distribution_class.py

flepimop/gempyor_pkg/tests/distributions/test_binomial_distribution_class.py

flepimop/gempyor_pkg/tests/distributions/test_beta_distribution_class.py

flepimop/gempyor_pkg/tests/distributions/test_truncatednormal_distribution_class.py

flepimop/gempyor_pkg/tests/distributions/test_poisson_distribution_class.py

TimothyWillard · 2025-07-14T15:48:56Z

Looking basically right. A few definite TODOs, but two items for discussion?

@emprzy @TimothyWillard what are your thoughts re 1) testing the initializer and 2) testing the output ranges?

depending how those opinions go, there's a bit of stripping out to do alongside the other fixes.

Re:

I don't think we should be in the business of testing vendor libraries, but the tests are already written so the change would be remove them which I also don't see a lot of value in. I don't have a strong opinion on what to do here, but agree we do not want to test pydantic/numpy/etc. going forward.
Yeah, that's fine to skip testing ranges with me, relates to (1). Might want to keep some for situations where we do something unconventional like the truncated normal (i.e. we modify the inputs to match our preferred input spec on ranges).

emprzy · 2025-07-14T15:55:54Z

Looking basically right. A few definite TODOs, but two items for discussion?
@emprzy @TimothyWillard what are your thoughts re 1) testing the initializer and 2) testing the output ranges?
depending how those opinions go, there's a bit of stripping out to do alongside the other fixes.

Re:

I don't think we should be in the business of testing vendor libraries, but the tests are already written so the change would be remove them which I also don't see a lot of value in. I don't have a strong opinion on what to do here, but agree we do not want to test pydantic/numpy/etc. going forward.

Yeah, that's fine to skip testing ranges with me, relates to (1). Might want to keep some for situations where we do something unconventional like the truncated normal (i.e. we modify the inputs to match our preferred input spec on ranges).

Agree about item 1).

Item 2), It would be pretty easy to add tests for ranges, if we want to do that. I see no harm in implementing them if we think it would add value.

Move shape tests to something that is tested at the `DistributionsABC` level, small verbiage changes in parametrization, re-naming of `DistributionsABC` testing file.

flepimop/gempyor_pkg/src/gempyor/distributions.py

flepimop/gempyor_pkg/tests/distributions/test_poisson_distribution_class.py

flepimop/gempyor_pkg/src/gempyor/distributions.py

flepimop/gempyor_pkg/tests/distributions/test_uniform_distribution_class.py

flepimop/gempyor_pkg/tests/distributions/test_truncatednormal_distribution_class.py

flepimop/gempyor_pkg/tests/distributions/test_distributions_common.py

flepimop/gempyor_pkg/src/gempyor/distributions.py

flepimop/gempyor_pkg/tests/distributions/test_poisson_distribution_class.py

flepimop/gempyor_pkg/tests/distributions/test_truncatednormal_distribution_class.py

TimothyWillard

Looks good to me

pearsonca · 2025-07-17T14:24:23Z

flepimop/gempyor_pkg/src/gempyor/distributions.py

+    n: int = Field(..., gt=0)
+    p: float = Field(..., gt=0.0, lt=1.0)


mmm, shouldn't these be ge, le still? and the == part is now being checked by the edge cases bit?

Do you not want n == 0 and (p == 0 or p == 1) to be considered an edge case?

yes - but isn't gt / lt going to strictly disallow them, irrespective of allow_edge_cases?

pearsonca · 2025-07-17T14:25:22Z

flepimop/gempyor_pkg/tests/distributions/test_binomial_distribution_class.py

should be exercising the allowed vs disallowed edge cases, right?

oh hm great point. I'll fix that.

Change to correct operator (`ge` and `le`), update tests accordingly

emprzy self-assigned this Jun 23, 2025

emprzy marked this pull request as ready for review June 23, 2025 15:53

emprzy requested review from MacdonaldJoshuaCaleb, TimothyWillard and pearsonca June 23, 2025 15:53

TimothyWillard reviewed Jun 23, 2025

View reviewed changes

flepimop/gempyor_pkg/src/gempyor/distributions.py Outdated Show resolved Hide resolved

flepimop/gempyor_pkg/src/gempyor/distributions.py Outdated Show resolved Hide resolved

TimothyWillard approved these changes Jun 25, 2025

View reviewed changes

flepimop/gempyor_pkg/src/gempyor/distributions.py Show resolved Hide resolved

TimothyWillard force-pushed the dev branch from 00a262d to 6722c3f Compare June 25, 2025 14:36

TimothyWillard linked an issue Jun 30, 2025 that may be closed by this pull request

Convert as_random_distribution To Use scipy.stats Distributions #290

Closed

TimothyWillard added enhancement Request for improvement or addition of new feature(s). gempyor Concerns the Python core. medium priority Medium priority. next release Marks a PR as a target to include in the next release. labels Jun 30, 2025

emprzy force-pushed the gempyor-distributions branch from 21b28c4 to 74713e2 Compare June 30, 2025 19:34

TimothyWillard and others added 15 commits June 30, 2025 17:44

Add UniformDistribution

3413d85

Added a representation for uniform distributions to `gempyor.distributions`.

Add PoissonDistribution

33bc456

Added a representation of a poisson distribution to `gempyor.distributions`

Add BinomialDistribution

bf06e4a

Added a representation for binomial distribution to `gempyor.distributions`.

Add LognormalDistribution

44d02c5

Added a lognormal distribution to `gempyor.distributions`.

Add TruncatedNormalDistribution

1ebc8ac

Added a truncated normal distribution to `gempyor.distributions`.

Add GammaDistribution

d573954

Added a representation for gamma distributions to `gempyor.distributions`

Add WeibullDistribution

0ce1aa0

Added a representation for Weibull distributions to `gempyor.distributions`. Also, fixed a return type hint error.

Add tests for NormalDistribution

5658993

Unit tests and validation for the `NormalDistribution` class in `gempyor_pkg/tests/distributions/`

Add tests for FixedDistribution

65093e3

Unit tests and validation for the `FixedDistribution` class in `gmepyor_pkg/tests/distributions/`. Also removal of an unused parameter in the `FixedDistribution` `.sample()` method.

Add tests for UniformDistribution

c530c97

Unit tests and validation for the `UniformDistribution` class in `gempyor_pkg/tests/distributions/`. Also linting from a previous commit I forgot to add.

Add tests for LognormalDistribution

dd21f04

Unit tests and validation for the `LognormalDistribution` class in `gempyor_pkg/tests/distributions/` + a fix for an issue that I created in `2c5fce5`

Add tests for TruncatedNormalDistribution

0f0c8eb

Add tests for the `TruncatedNormalDistribution` class from `gempyor.distributions`. Also, formatting fixes and bound fixes.

Add tests for PoissonDistribution

fa71562

Unit tests and validation for `PoissonDIstribution` class in `gempyor.distributions`. Also, formatting and bounds fixes.

Reformatting with black

bc1edee

TimothyWillard reviewed Jul 10, 2025

View reviewed changes

flepimop/gempyor_pkg/src/gempyor/distributions.py Show resolved Hide resolved

emprzy added 2 commits July 11, 2025 11:47

Refactor tests for new ABC rng property.

8f7daf8

Reduce chance transient sampling error in test_distributions_abc.py

3b75ca5

Take 10 samples rather than 1 to reduce the risk of an identical series of samples being drawn (causing a test failure when arrays are compared).

TimothyWillard reviewed Jul 11, 2025

View reviewed changes

flepimop/gempyor_pkg/src/gempyor/distributions.py Outdated Show resolved Hide resolved

TimothyWillard reviewed Jul 14, 2025

View reviewed changes

flepimop/gempyor_pkg/src/gempyor/distributions.py Outdated Show resolved Hide resolved

pearsonca reviewed Jul 14, 2025

View reviewed changes

flepimop/gempyor_pkg/src/gempyor/distributions.py Outdated Show resolved Hide resolved

Apply suggestions from code review

711d25f

use pydantic capabilities for default_factory Co-authored-by: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com>

pearsonca reviewed Jul 14, 2025

View reviewed changes

emprzy added 3 commits July 14, 2025 17:46

Abstract shape tests into test_distributions_common.py

69756eb

Move shape tests to something that is tested at the `DistributionsABC` level, small verbiage changes in parametrization, re-naming of `DistributionsABC` testing file.

Typo fix and additional bounds test for TruncatedNormalDistribution

1e32b6d

Build parameter allow_edge_cases: bool = False to distribution classes

d5a5e98

emprzy requested review from TimothyWillard and pearsonca July 15, 2025 20:08

TimothyWillard reviewed Jul 16, 2025

View reviewed changes

flepimop/gempyor_pkg/tests/distributions/test_poisson_distribution_class.py Show resolved Hide resolved

TimothyWillard reviewed Jul 16, 2025

View reviewed changes

flepimop/gempyor_pkg/tests/distributions/test_truncatednormal_distribution_class.py Show resolved Hide resolved

Apply suggestions from @TimothyWillard 's review

8185916

emprzy requested a review from TimothyWillard July 16, 2025 19:24

TimothyWillard approved these changes Jul 17, 2025

View reviewed changes

pearsonca requested changes Jul 17, 2025

View reviewed changes

Change Field constraints for BinomialDistribution

921f41a

Change to correct operator (`ge` and `le`), update tests accordingly

emprzy merged commit d038d78 into dev Jul 17, 2025
11 checks passed

emprzy deleted the gempyor-distributions branch July 17, 2025 16:35

emprzy mentioned this pull request Jul 17, 2025

[Feature request]: Gamma and Weibull Distribution support #573

Closed

		n: int = Field(..., gt=0)
		p: float = Field(..., gt=0.0, lt=1.0)

Conversation

emprzy commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes.

Does this pull request make any user interface changes? If so please describe.

What does your pull request address? Tag relevant issues.

Uh oh!

TimothyWillard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emprzy commented Jun 24, 2025

Uh oh!

TimothyWillard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TimothyWillard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pearsonca left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimothyWillard commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emprzy commented Jul 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimothyWillard left a comment

Choose a reason for hiding this comment

Uh oh!

pearsonca Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

emprzy Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

pearsonca Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

pearsonca Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

emprzy Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emprzy commented Jun 20, 2025 •

edited

Loading

TimothyWillard left a comment •

edited

Loading

pearsonca left a comment •

edited

Loading

TimothyWillard commented Jul 14, 2025 •

edited

Loading