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

Unify Reals,Bint into Array #356

Merged
merged 3 commits into from
Aug 18, 2020
Merged

Unify Reals,Bint into Array #356

merged 3 commits into from
Aug 18, 2020

Conversation

fritzo
Copy link
Member

@fritzo fritzo commented Aug 18, 2020

Addresses #322, #351 , #348

This unifies BintType and RealsType types into a common ArrayType with support for .dtype and .shape (I have unmarked those deprecated). After this PR Bint[size, i,j,k] denotes a homogeneously-bounded i x j x k-sized array of Bint. This PR does not yet use those integer valued arrays in distributions or elsewhere; that is for follow-up.

This PR also clarifies the role of Bint. Whereas I had previously thought we would need heterogeneously-bounded distributions (as e.g. requested of dist.Multinomial), I now believe we can restrict to homogeneously-bounded Bint types, and add a separate heterogeneous type Tuple[Bint[2], Bint[3]) for indexing into arrays. This PR does not add that Tuple, but it does clarify the planned type hierarchy:

Domain
+-- ArrayType                                 \
|   +-- RealsType                              }  support .dtype, .shape
|   +-- BintType  # homogeneously bounded     /
+--TupleType      # heterogeneously bounded   }   does not support .dtype, .shape

Tested

  • pure refactoring is covered by existing tests

Copy link
Member

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

Looks good per our discussion earlier

@fritzo fritzo merged commit f712c68 into master Aug 18, 2020
@fritzo fritzo deleted the array-type branch August 18, 2020 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants