Prepare for generalized outcomes

ihincks commented 8 years ago

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 commented 8 years ago

62 files changed 95 commits

taalexander commented 8 years ago

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 https://github.com/QInfer/python-qinfer/pull/71#issuecomment-240251337, or mute the thread https://github.com/notifications/unsubscribe-auth/ABe1tqbKQujPtyeumVjwCLcg9Jm8XzMyks5qgjFMgaJpZM4Jly8n .

cgranade commented 8 years ago

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 commented 8 years ago

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

git merge upstream/master

crosses fingers

ihincks commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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.

cgranade commented 8 years ago

After thinking on it for the weekend, I think the best way forward with this PR is for me to git cherry-pick the QuTiP CI configuration and 3.2 requirements into a separate PR so that I can test them separately. If that works, that will make the rest of the PR much easier to merge in.

ihincks commented 8 years ago

Okay, added a skipIf to the TestTomographyModel. Thanks for pointing out this feature, @cgranade .

coveralls commented 8 years ago

Coverage increased (+12.09%) to 57.232% when pulling 579b08b76f407b5aa4b8fe1e9095457b9ecaacd7 on ihincks:feature-prepare-for-generalized-outcomes into acfa62fc111780c682fa8b50235130ea701e90b1 on QInfer:master.

coveralls commented 8 years ago

Coverage increased (+12.09%) to 57.232% when pulling 579b08b76f407b5aa4b8fe1e9095457b9ecaacd7 on ihincks:feature-prepare-for-generalized-outcomes into acfa62fc111780c682fa8b50235130ea701e90b1 on QInfer:master.

cgranade commented 8 years ago

I think this looks really good! Thanks for everything with this PR. I want to go through in a bit more detail, as well as discuss w/ @csferrie, but I think this is nearly ready to merge in.

coveralls commented 8 years ago

Coverage increased (+12.09%) to 57.232% when pulling d0814e0b318838d81f10d3260abf4cc46c0e2c56 on ihincks:feature-prepare-for-generalized-outcomes into acfa62fc111780c682fa8b50235130ea701e90b1 on QInfer:master.

coveralls commented 8 years ago

Coverage increased (+12.09%) to 57.232% when pulling d0814e0b318838d81f10d3260abf4cc46c0e2c56 on ihincks:feature-prepare-for-generalized-outcomes into acfa62fc111780c682fa8b50235130ea701e90b1 on QInfer:master.

cgranade commented 8 years ago

Looking in more detail, I had a bit of confusion about the roles of the new properties and methods (in particular, domain) in relation to previous interfaces (e.g.: n_outcomes). Would you be willing to add a bit to doc/guide/models.rst describing the changed interfaces? If not, I'm also happy to work my way through it and write said docs. Thanks for the huge contribution!

ihincks commented 8 years ago

The role of n_outcomes is definitely something that deserves attention. In this PR, n_outcomes is completely independent of domain. I will explain in detail later. In short, part of the problem is wanting to keep a property called n_outcomes in the new system that means for FiniteOutcomeModel what it did for Model before this PR. For finite-outcome models, it will typically hold that n_outcomes(expparam)=domain(expparam).n_members. For models with an infinite number of outcomes (or a very large number of outcomes such that enumerating them is painful), n_members is not defined, but n_outcomes sort of refers to "enough outcomes" to make estimates of properties like Bayes' risk.

ihincks commented 8 years ago

(But yes, I will add to the guide)

cgranade commented 8 years ago

Ah, that makes a lot more sense, then. Thank you for the explanation, and for all the help!

coveralls commented 8 years ago

Coverage increased (+12.08%) to 57.274% when pulling 4d40b7f611e1b530584c8e6a783c0c0286e4f27e on ihincks:feature-prepare-for-generalized-outcomes into de46ca5a63dd0a6cc547f9065be13e997216f778 on QInfer:master.

coveralls commented 8 years ago

Coverage increased (+12.02%) to 57.211% when pulling 182a9c917d7f7300edadda9fa499e8051b749029 on ihincks:feature-prepare-for-generalized-outcomes into de46ca5a63dd0a6cc547f9065be13e997216f778 on QInfer:master.

coveralls commented 8 years ago

Coverage increased (+12.02%) to 57.211% when pulling 182a9c917d7f7300edadda9fa499e8051b749029 on ihincks:feature-prepare-for-generalized-outcomes into de46ca5a63dd0a6cc547f9065be13e997216f778 on QInfer:master.

scasagrande commented 8 years ago

Py2.7 is having a problem due to the print statement that I have commented on. Conda 3.5 has an issue with the doctests.

As a reminder though for everyone, a PR of this magnitude is impossible to code review (and thus, I haven't looked over the actual implementation). Future work should be done in as small of chunks as possible. It keeps thing reviewable, reduces churn, and helps prevent the problem where peoples' local code branches too heavily from master, creating massive merge conflicts.

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

ihincks commented 8 years ago

Thanks for catching that forgotten print statement @scasagrande.

I agree that this PR is big. At the same time, it really does affect all of these files: if you add some methods and properties to a fundamental abstract class, you gotta fix everything that inherits from it. And then I introduced more than 100 tests involving all inheriting classes to try to minimize errors.

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

scasagrande commented 8 years ago

There is always ways to split the work up, and part of learning to work on a code base with a team is submitting code-review-able PRs. This might involve a number of PRs merged into a staging branch before merging into a primary branch.

Some ideas on how to slice large base changes like this:

Whitespace. If your project doesn't have a set of linting rules you'll probably have whitespace changes. If you know your work going to result in a lot of line changes, get this out of the way first so as to not overwhelm people with green lines.
Tests for existing functionality / APIs. These tests can be useful even if you don't make your other functionality changes, so go ahead and make them a separate PR
Base class changes, and minimum class changes. I'd skip class-specific implementations at this point. This can go into a staging branch.
Class specific implementations. Whether this is one PR or one for each class depends on how intense the changes are. For this work, probably just one.

To give you an idea, at work, the worst case we had was when we had to rewrite this massive file. It was pure refactoring work, but had to be done so that our actual work could be implemented (so one degree of PR-splitting already). We had to change it from being some all-knowing god-object into using proper dependency injection. We split it as much as we could (one PR for new tests, one for the refactoring) but it was still ugly to review.

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

coveralls commented 8 years ago

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

QInfer / python-qinfer

Prepare for generalized outcomes #71