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

Prepare for generalized outcomes #71

Merged

Conversation

ihincks
Copy link
Collaborator

@ihincks ihincks commented Aug 16, 2016

This is a beefy PR addressing issue #66. The nutshell improvement is that we want experiment outcomes to be able to have fancy data types (like expparams), and to be compatible with future improvements that lift the constraint that any experiment must have a finite number of possible outcomes (this constraint comes in the form of some methods like bayes_risk enumurating over all possible outcomes).

This is handled as follows:

  • new abstract class called Domains, not much to it, just tells you stuff like whether it's finite and is able to spit out values in the domain. This comes with obvious subclasses like IntegerDomain and RealDomain.
  • method domain(self, expparam) of Simulateable, returning a Domain for every set of expparms.
  • subclass FiniteOutcomeModel of Model which can have outcome-enumerating methods
  • some other helper properties and methods that you don't usually need to worry about (ex: are_expparam_dtypes_consistent tells you if all the expparams correspond to the same datatype)

Handling breaking changes:

There are two breaking changes, and they're both very easy to fix on existing models. You need to inherit from FiniteOutcomeModel instead of Model, and you need to implement the domain method, returning for example IntegerDomain(min=0, max=1) for a two outcome model. Another thing that comes up is that Model.pr0_to_likelihood_array is moved to FiniteOutcomeModel.pr0_to_likelihood_array.

New Features

  • Model testing: Most of the lines of code added in this PR come from a new systematic set of tests for models. There is now a set of tests for every single model and derived model defined in qinfer. These tests do not do anything inference theoretic, they mainly test that models are type-consistent, shape-consistent, fully-implemented, and bug free.
  • As a proof of principle, I implemented a MultinomialModel (which requires fancy outcome data type which is a tuple of results) which generalizes BinomialModel.

TODO

  • I went through all files in qinfer and tried to make sure they were consistent with this PR. To my knowledge, the only remaining things to fix are bugs not caught by testing.
  • However, I did not touch the documentation (I did comment code very well).

NOTES

I pulled in the feature-improved-docs because it would be confusing for me not to.

@scasagrande
Copy link
Collaborator

62 files changed
95 commits

@taalexander
Copy link
Contributor

Great work on the PR Ian. Ian has made fairly reasonable arguments to me
that these changes should be included in V1.0

On Tue, Aug 16, 2016 at 5:53 PM, Steven Casagrande <notifications@github.com

wrote:

62 files changed
95 commits

https://camo.githubusercontent.com/668dd8d6000fc37a8d408433d16618459ff07507/68747470733a2f2f63646e2e6d656d652e616d2f696e7374616e6365732f35343333393832342e6a7067


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#71 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABe1tqbKQujPtyeumVjwCLcg9Jm8XzMyks5qgjFMgaJpZM4Jly8n
.

@cgranade
Copy link
Collaborator

cgranade commented Sep 1, 2016

Now that I'm back from travel, trying to catch up on PRs, I apologize for the delay and thank you for your wonderful contribution. master should now be much more up to date after merging in the other PRs; could you please merge those in? Thank you again! ♥

@ihincks
Copy link
Collaborator Author

ihincks commented Sep 1, 2016

Thanks Chris! Hopefully this won't be too painful. I'll get on it.

git merge upstream/master

crosses fingers

…or-generalized-outcomes

Conflicts:
	.gitignore
@ihincks
Copy link
Collaborator Author

ihincks commented Sep 1, 2016

Oh, that was actually pretty easy. The tests seem to be failing because qutip isn't being imported in one of the new tomography model tests and so what should be the qutip module is actually None. I don't presently know enough about how these tests work to fix this...they pass on my computer.

This PR should probably also include a guide on outcomes. I was pretty careful going through inline documentation making sure it still made sense, but it's possible some of the examples or guides need to deal with outcomes.

@ihincks
Copy link
Collaborator Author

ihincks commented Sep 1, 2016

@whitewhim2718 pointed out that in tomography, version >=3.1 of qutip was required. However, in version 3.1, functions like rand_dm_ginibre do not exist. The newest commit demands qutip version >=3.2 across the board for use with tomography.

I'm not sure if this is the right thing to do because 3.1 is the highest official release of qutip, though it is quite old (2014).

This fix is independent of travis-ci not having qutip at all.

@cgranade
Copy link
Collaborator

cgranade commented Sep 1, 2016

I think depending on QuTiP 3.2 prereleases is fine, since as you point out, it is quite old, and as the official 3.2 is scheduled to come out very soon. In particular, many of the features such as rand_dm_ginibre were contributed to QuTiP precisely for QInfer tomography support.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling 1dd6148 on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling c4d2bd1 on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling c4d2bd1 on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling c4d2bd1 on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling c4d2bd1 on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling f9248e5 on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling f9248e5 on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling f9248e5 on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling 32fca7b on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling 32fca7b on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling 32fca7b on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

coveralls commented Sep 9, 2016

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling 32fca7b on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@ihincks
Copy link
Collaborator Author

ihincks commented Sep 9, 2016

Okay @cgranade, I have modified the models.rst guide to have a section on outcomes, and to not fail due to Model vs FiniteOutcomeModel. I also have included some more API docs for Domains, MultinomialModel, etc.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling 871018b on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling 871018b on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling 871018b on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling 871018b on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@coveralls
Copy link

coveralls commented Sep 9, 2016

Coverage Status

Coverage increased (+12.006%) to 57.198% when pulling 871018b on ihincks:feature-prepare-for-generalized-outcomes into ea90440 on QInfer:master.

@cgranade
Copy link
Collaborator

Wonderful, thank you so much @ihincks! I've looked over the documentation, and it all builds fine, looks great, so thank you as well for adding that. Since it all passes now, I think it's ready to merge in. Thank you again!

@cgranade cgranade merged commit 7fa6d17 into QInfer:master Sep 13, 2016
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.

None yet

5 participants