Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove custom standard RV Op(s) and add remaining "standard" NumPy RVs #1461

Merged
merged 3 commits into from
Mar 10, 2023

Conversation

Smit-create
Copy link
Member

Fixes #1358

Here are a few important guidelines and requirements to check before your PR can be merged:

  • There is an informative high-level description of the changes.
  • The description and/or commit message(s) references the relevant GitHub issue(s).
  • pre-commit is installed and set up.
  • The commit messages follow these guidelines.
  • The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
  • There are tests covering the changes introduced in the PR.

@Smit-create Smit-create added enhancement New feature or request random variables Involves random variables and/or sampling NumPy compatibility Op implementation Involves the implementation of an Op labels Mar 2, 2023
Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

We really need to do something about the existing StandardNormalRV (i.e. #530), because I believe the new class approach was taken due to its convenience with regard to RandomStream (i.e. it works with the current RandomStream.__getattr__ implementation). See #528.

In general, we don't want to add new Op classes if we don't have to, especially in cases like this where the new Op type implies that its corresponding Apply nodes will have specific inputs, but there is nothing enforcing this constraint. One reason this approach is problematic: an Apply using a StandardNormalRV Op can be rewritten to have non-zero mean and/or non-unit variance. This implies that StandardNormalRV is redundant at best, but can also be misleading. (Likewise, adding input constraints to fix this wouldn't help our graph representations in any foreseeable way; if anything, it would unnecessarily complicate things.)

That said, we should first see what can be done about making RandomStream work with simple definitions like standard_normal = normal(0, 1) and the partial functions.

@Smit-create Smit-create force-pushed the i-1358 branch 2 times, most recently from 6093a25 to 51b1aad Compare March 3, 2023 17:00
@Smit-create
Copy link
Member Author

Ah, the failure on CI is hard to reproduce on my machine. It says:

FAILED tests/link/jax/test_random.py

But pytest does pass for that on my setup.

@brandonwillard
Copy link
Member

brandonwillard commented Mar 4, 2023

But pytest does pass for that on my setup.

This is often due to version mismatches between local environments and CI.

I think this issue appeared in another PR as well, so you can add a commit marking it as xfail for the time being and we'll get back to it later/elsewhere.

@brandonwillard
Copy link
Member

The issue should be fixed in #1464.

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #1461 (60445b7) into main (c433071) will increase coverage by 0.01%.
The diff coverage is 91.42%.

❗ Current head 60445b7 differs from pull request most recent head ffa4d81. Consider uploading reports for the commit ffa4d81 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1461      +/-   ##
==========================================
+ Coverage   74.86%   74.87%   +0.01%     
==========================================
  Files         194      194              
  Lines       50107    50129      +22     
  Branches    12098    12097       -1     
==========================================
+ Hits        37514    37536      +22     
  Misses      10266    10266              
  Partials     2327     2327              
Impacted Files Coverage Δ
aesara/compile/profiling.py 74.55% <ø> (ø)
aesara/link/jax/dispatch/random.py 100.00% <ø> (ø)
aesara/tensor/basic.py 89.99% <ø> (ø)
aesara/tensor/elemwise.py 88.07% <ø> (ø)
aesara/tensor/var.py 87.82% <50.00%> (ø)
aesara/tensor/rewriting/math.py 86.11% <60.00%> (ø)
aesara/link/jax/dispatch/scalar.py 96.72% <100.00%> (ø)
aesara/link/numba/dispatch/elemwise.py 97.12% <100.00%> (ø)
aesara/scalar/basic.py 79.17% <100.00%> (-0.01%) ⬇️
aesara/tensor/inplace.py 100.00% <100.00%> (ø)
... and 2 more

@Smit-create Smit-create marked this pull request as ready for review March 9, 2023 11:27
@brandonwillard brandonwillard changed the title [WIP] Add standard RVs Remove custom standard Ops and add remaining NumPy entries Mar 9, 2023
@brandonwillard brandonwillard changed the title Remove custom standard Ops and add remaining NumPy entries Remove custom standard Ops and add remaining "standard" NumPy RVs Mar 9, 2023
@brandonwillard brandonwillard changed the title Remove custom standard Ops and add remaining "standard" NumPy RVs Remove custom standard RV Op(s) and add remaining "standard" NumPy RVs Mar 9, 2023
Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

I added a fix for the use of partial functions in RandomStream and rebased. This should be good to merge once the tests pass.

@brandonwillard brandonwillard enabled auto-merge (rebase) March 9, 2023 23:11
@brandonwillard brandonwillard merged commit 18f65fd into aesara-devs:main Mar 10, 2023
@Smit-create Smit-create deleted the i-1358 branch March 10, 2023 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request NumPy compatibility Op implementation Involves the implementation of an Op random variables Involves random variables and/or sampling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add standard_cauchy, standard_gamma, standard_t and standard_exponential RandomVariable
2 participants