Add Minipyro Example #8
Conversation
|
This should close #2 |
There was a problem hiding this comment.
Looks great! I requested several mostly cosmetic changes below which are somewhat in the weeds, but which I think are worth addressing to make this example more effective as documentation for the library.
We can meet tomorrow and go through the more substantive design issues in the library that this exercise has uncovered.
tests/conftest.py
Outdated
| try: | ||
| item.runtest() | ||
| except NotImplementedError as e: | ||
| pytest.xfail(str(e)) |
There was a problem hiding this comment.
Which tests imported from pyroapi fail without this?
There was a problem hiding this comment.
- tests which use the jit
- tests which use clipped ADAM
- tests which use
TraceMeanField_ELBO - Tests which use the
event_dimarg ofplate
There was a problem hiding this comment.
OK, can we make this more specific so that it only xfails the failing minipyro tests? Otherwise this will apply to all current and future tests in effectful.
There was a problem hiding this comment.
Unfortunately, the pyro-api package is the one that raises the exceptions (which are just NotImplementedErrors), not us, so it's difficult to track the cause of each issue. I can do a check based on substrings which works for now, but it's a bit of a hack.
There was a problem hiding this comment.
A check based on substrings is OK for now. You might also be able to restrict this to only apply to tests in test_minipyro.py, so that it doesn't inadvertently xfail tests in other parts of the library.
eb8680
added
documentation
|
Can you run |
This re-implements the minipyro example from pyro on top of the effectful library. The pyro-api tests for the parts we've implemented are all passing.