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

Convert to PyTest #333

Merged
merged 3 commits into from Feb 14, 2020
Merged

Convert to PyTest #333

merged 3 commits into from Feb 14, 2020

Conversation

Lnaden
Copy link
Contributor

@Lnaden Lnaden commented Feb 14, 2020

Drops the support for Nose and replaces all the tests with the PyTest module, including the CI

Fixes #330

@Lnaden Lnaden added the tests label Feb 14, 2020
@Lnaden Lnaden added this to the 4.0 release milestone Feb 14, 2020
@Lnaden
Copy link
Contributor Author

Lnaden commented Feb 14, 2020

I've also disabled the old AppVeyor and Travis hooks for the future past commit b9afae4


"""Can MBAR calculate E(u_kn)??"""

for system_generator in system_generators:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was system_generators iterating over? Is it important that it test multiple systems?

Copy link
Collaborator

@mrshirts mrshirts Feb 14, 2020

Choose a reason for hiding this comment

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

OK, I checked - it's validating both harmonic oscillators and exponential distributions. Is it still doing that?

Copy link
Contributor Author

@Lnaden Lnaden Feb 14, 2020

Choose a reason for hiding this comment

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

Yes, it is still doing that as per this bit:

https://github.com/choderalab/pymbar/pull/333/files#diff-d3d216b6c9c7ae91ac28ab96198e2cf0R42-R54

and this bit:

https://github.com/choderalab/pymbar/pull/333/files#diff-d3d216b6c9c7ae91ac28ab96198e2cf0R147

I'm using PyTest to make a parameterized fixture so as only to call the MBAR creation routine once per generator (which I am using as the parameterization on the fixture itself. That way, each test is not re-creating the MBAR object for every test and generator combination.

PyTest does some magic with decorators and its Fixtures and Parameterization, they start warping the standard function calls a bit, but it turns out to be really powerful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output of the test starts looking like this:

pymbar/tests/test_mbar.py::test_mbar_computeMultipleExpectations[generate_ho] PASSED [ 17%]
...
pymbar/tests/test_mbar.py::test_mbar_computeMultipleExpectations[generate_exp] PASSED [ 37%]

Where each parameterization ran through is shown in the [...]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hah, I'm just having a hard time figuring out how it can generate both of the sets without running. What is the syntax for autorunning the pytests?

Copy link
Contributor Author

@Lnaden Lnaden Feb 14, 2020

Choose a reason for hiding this comment

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

Im not quite sure I follow, but they are running, and here is my loose understanding of what PyTest is doing for its magic:

  1. PyTest is invoked on the tests directory, it starts doing some regex matching for collecting tests (a bunch of other config things too)
  2. As modules start up, PyTest starts looking for anything decorated with the pytest.fixture decorator, which the function mbar_and_test is one of them.
  3. PyTest then looks through all the functions matching the regex and their signatures for any parameter matching the name of the fixtures. In this case mbar_and_test. So def test_mbar_free_energies(mbar_and_test) matches.
  4. Anytime PyTest now calls a function with a fixture it knows about, it will auto replace that function call when executing the test with the fixture.
  5. Because the scope='module' option is set, PyTest will know to only execute the fixture once per module
  6. Because I also set params=system_generators in pytest.fixture, PyTest will also cycle every item in the params to the fixture, which uses the PyTest special global fixture request to access. Since there are 2 items, 2 instances of the fixture get cycled every time its used in a test later.

Now that all the prep is done, here is what happens:

  1. Test is called, it uses the fixture called mbar_and_test which has been instanced twice for the module,
  2. Test is run with the yield'ed objects from that fixture (I could have return'ed them too here)
  3. Test is run again with the next yield'ed objects from that fixture because it was parameterized and referenced, so PyTest handles the combination.

Much shorter: PyTest is doing ALOT of function substitutions behind the scenes to really empower highly customizable and parametrizable testing suites behind the scenes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be more accurate of an answer to your question to say "There is no syntax to autorun fixtures and parameters beyond their decorators with PyTest, its just doing it all behind the scenes when invoking pytest"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, by "autorunning" I meant "manually running" so I could run and inspect output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is one downside to this, is you're not supposed to invoke fixtures manually, and it was removed from version 4.0 of pytest. If you pass in an object which looks like what the fixture returns, you can still call the function by hand. But there are plenty of tools to engage with individual tests with PyTest itself if thats what you need.

  • You can manually specify individual tests, or patterns of them from pytest's CLI with the -k flag: e.g. pytest -k test_sa will only run tests and any needed fixtures/parameters matching test_sa* as a function name
  • You can add a bunch of print statements and do -s flag to make the output non-capturing of strings and print them all
  • You can add --pdb and Pytest will drop into the Python Debugger at the line which failed on first and any fail
  • You can insert a pdb.set_trace() or breakpoint in the tests themselves and it will happily stop there
  • You can mix and match all of the above and I'm sure there are a bunch Im missing too.

I personally have not had a case where I've needed to manually run a test outside of PyTest where I could not manipulate the tests in some way and didn't get what I needed. If you want I can make a non-fixture version of that function to give back the ability to manually run it, although you would still have to have some way to interrupt it, and then its just back to the list from above?

I don't know if that covers the use case you're describing, so please let me know if I'm still misinterpreting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That should give me the information I need! I'lll work on this over the next day or two.

Lnaden added a commit to Lnaden/pymbar that referenced this pull request Feb 14, 2020
This PR is an extension of choderalab#333, so it is DNM until 333 is done

This PR replaces all of the `np.matrix` calls and manipulates the
objects as arrays only. See https://numpy.org/doc/1.18/user/numpy-for-matlab-users.html
for why this needed to be done.

However, that does mean there are some syntax changes I had to make since
`*` on arrays is element-wise (matrix multiplication can be done with
`@` operator since Python 3.5, though you can do `np.matmul(a,b)` to
the same effect ).
And transpose of 1-D arrays doesn't do the correct thing, but I found
doing `x - np.astleast_2d(x).T` does, while still being arrays and not
matrices.

That said, there's alot of replacements here and I'm hoping someone
else can look over this and make sure I have the correct matrix operations
(namely element-wise vs matrix-style) in place for the correct results.
This does pass my local tests.
Copy link
Collaborator

@mrshirts mrshirts left a comment

Choose a reason for hiding this comment

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

OK, this runs and seem to be operating correctly. I'm fine with it being merged! In fact, I'll go ahead.

@mrshirts
Copy link
Collaborator

Merging new tests.

@mrshirts mrshirts merged commit 90666f6 into choderalab:pymbar4 Feb 14, 2020
Lnaden added a commit to Lnaden/pymbar that referenced this pull request Feb 17, 2020
This PR is an extension of choderalab#333, so it is DNM until 333 is done

This PR replaces all of the `np.matrix` calls and manipulates the
objects as arrays only. See https://numpy.org/doc/1.18/user/numpy-for-matlab-users.html
for why this needed to be done.

However, that does mean there are some syntax changes I had to make since
`*` on arrays is element-wise (matrix multiplication can be done with
`@` operator since Python 3.5, though you can do `np.matmul(a,b)` to
the same effect ).
And transpose of 1-D arrays doesn't do the correct thing, but I found
doing `x - np.astleast_2d(x).T` does, while still being arrays and not
matrices.

That said, there's alot of replacements here and I'm hoping someone
else can look over this and make sure I have the correct matrix operations
(namely element-wise vs matrix-style) in place for the correct results.
This does pass my local tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants