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

Feature: ProductDomain #117

Merged
merged 9 commits into from
Apr 7, 2017
Merged

Conversation

ihincks
Copy link
Collaborator

@ihincks ihincks commented Mar 19, 2017

This PR adds a new domain called ProductDomain which takes the cartesian product of any number of factor domains. This is done using numpy structured dtypes, so you can, for example, take the product of MultinomialDomain and RealDomain and end up with a dtype like [('k', int, 3), ('f1', float)].

This PR should be merged after #116 and #115.

Copy link
Collaborator

@cgranade cgranade left a comment

Choose a reason for hiding this comment

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

There's a couple warnings being raised by pylint on Travis about Python 3 compatibility. Checking those against py.test on 3.5, these two warnings may well be what's causing test failures. I've added notes below. Thanks!

:type: ``int`` or ``np.inf``
"""
if self.is_finite:
return reduce(mul, [domain.n_members for domain in self._domains], 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is raising a Py3 compatibility warning; should be able to fix by importing reduce from functools.

# taken from http://stackoverflow.com/questions/5355744/numpy-joining-structured-arrays
sizes = np.array([a.itemsize for a in arrays])
offsets = np.r_[0, sizes.cumsum()]
shape = arrays[0].shape
Copy link
Collaborator

Choose a reason for hiding this comment

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

arrays is being called with the result of map in line 238 of domains.py, but map is not indexible on Python 3. I'd suggest calling list(map(...)) above since that also clears the Py3 warning that's blocking the Travis build.

@coveralls
Copy link

coveralls commented Mar 31, 2017

Coverage Status

Coverage increased (+3.2%) to 72.351% when pulling 28ed55f on ihincks:feature-product-domain into 12b2612 on QInfer:master.

Copy link
Collaborator

@cgranade cgranade left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, the changes look good and the PR now passes CI, so I'll go on and merge it in.

@cgranade cgranade merged commit 4d0aee3 into QInfer:master Apr 7, 2017
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.

3 participants