Write tests
Closed this issue · 15 comments
Work with this basic example: 3 states with a random transition matrix.
- stationary
- time-dependent examples: finite time (at least 3 time points) and
- periodic (at least 2 timepoints)
To check in the tests:
stationary case
- committors should be between 0 and 1
- backward transition matrix should be a stochastic matrix
- stationary distribution should be the stationary distribution of P and P^-
- check nan values.
- normalized reactive density should be normalized.
- check shape of returned ndarrays.
- test agains the transition matrix used for the small network
periodic case
- committors should be between 0 and 1
- backward transition matrix should be a stochastic matrix
- periodic distributions should be fit to P and P^-
- check nan values.
- normalized reactive density should be normalized.
- check shape of returned ndarrays.
- make M a variable (the length of the period)
- test agains the transition matrix used for the small network
finite-time
- committors should be between 0 and 1
- when a distribution is given, distributions are later times fit to the transition matrices
- check nan values.
- normalized reactive density should be normalized.
- check shape of returned ndarrays.
- make N a variable (the length of the interval)
- test agains the transition matrix used for the small network
- check irreducibility of transition matrices (Enric)
general issues for tests:
- time spend in each method. Decorator not needed, it suffices the flag '--durations' for pytest. (Enric)
- test fixtures: add random number generator seed and ensure that fixtures are only set once per test class
broadcasting
- check the vectorized methods in the code with non-vectorized tests (to do: finite case).
To check in the implementation:
- code is vectorized (numpy) (Enric)
- add check ups that A and B are disjoint, non-empty and their union is not S (Luzie)
- split up computation of q_f and q_b? (Luzie) - only for stationary and periodic, do we also want it for the finite-time class?
- committor computation using sp.linalg.solve (stationary and periodic case)
- periodic and finite time class: check isfunction statement (Enric)
- check that input is of the right type (ie numpy array not matrix, not lists - if not, simply convert?) (Luzie)
- for stationary tpt class: check that input transition matrix is irreducible
- finite time class: do we want to split committor computation from computing backward transition matrix, then backward T could also be improved using vectorization
- delete _ from self._ everywhere
Documentation:
- pytest package: https://docs.pytest.org/en/latest/
- how to work with fixtures: https://docs.pytest.org/en/latest/fixture.html#fixtures
- skip tests (useful while the developement): http://doc.pytest.org/en/latest/skipping.html
- run the tests for different python versions: https://pypi.org/project/tox/
- good practices: https://docs.pytest.org/en/latest/goodpractices.html#use-tox
@eborrell Maybe we should change the 3-state stationary P matrix to be a bit more complicated, because now P=P_back (or was that on purpose)?
@LuzieH definetely! I have changed P for a random stochastic matrix and the dimension of the state space can be given as a parameter (see tests/conftest.py).
$ pytest (has S=100 as default), or
$ pytest --dim-state-space 1000 (has S=1000)
@LuzieH I have made a wiki page for the test docu and some commands. Have a look! :)
https://github.com/LuzieH/pytpt/wiki/Tests
@eborrell The Wiki looks great! Good idea to do that :)
And I realized one more thing regarding random P matrices: I just had a failed test_reac_density(self, small_network) since the normalized reactive density had nan values. When I ran it again it was successful. Maybe random matrices are not such a good thing, because one can't find out where the exact error comes from, it's not reproducible and depends on the random seed. My guess for why the test failed is that the random matrix wasn't irreducible (which I am not sure how to enforce?). But on a second thought, producing irreducible matrices is very likely since the sampled transition matrix with high probability has non-zero values in each entry. So then I don't know about the error?
What do you think? Do you have an explanation?
@eborrell when writing the tests for the finite time case, I defined the transition matrix function using functools.partial, but the class finite threw an error at this due to : assert isfunction(P) == True , I commented it out in the finite class, but have to find a solution for that. maybe isfunction is not general enough?
@LuzieH Regarding the failed test, I agree that we should also test the methods against a not random matrix.
For the tests with random matrices we could write a validator which tells us if a matrix is irreducible or not and then test that in the beginning and imply that if this fails the test should not continue (I think that this happens anyway.)
For the test with a given matrix I would impose that this matrix is irreducible.
Hey @LuzieH. Here a brief review :)
- I made a validator for irreducible matrices.
- I found how to load data in the tests (You need to install the package pytest-datadir). This allows us to actually test the small_network example and the triple well (therefore I deleted the pipelines test). I adapt the test so that if the flag --small-network is given we test against the small network example (same can be done with the triple well). Some refractoring has been done as well
- For the stationary case the reac_dens_test fails for the small_network due to the fact that the asserting with the nan values do not hold. I haven't check why.
- Regarding the time spend in each method, now we can see how much time each test needs by adding the flag --durations=0, which already gives us an idea if a method is fast or slow.
- I did not have time for continuing with the vectorization.
@eborrell
(1) I also get another new bug/warning (but I am not sure why it appears):
:6: DeprecationWarning: Calling nonzero on 0d arrays is deprecated, as it behaves surprisingly. Use atleast_1d(cond).nonzero() if the old behavior was intended. If the context of this warning is of the form arr[nonzero(cond)], just use arr[cond].
it shows up for all finite-time tests: ie.. test_transition_matrix, test_density, test_committors etc.
(2) I added assertion tests for the sets A,B,C in the stationary, finite, periodic tpt classes. They seem to work when I run the triplewell and small network example. But when I run pytest I get errors.
I really believe there is something going wrong with the new test systems :/
UPDATE: I found the bug (I hope this also solves the bug you mention above):
- The states of the small_network were saved as a dictionary, therefore the procedure to get ind_A, ind_B, ind_C didn't work and instead no indices were assigned (so basically A;B,C were empty sets). this also solves my issues (1) and (2) as mentioned above.
- Another thing I realized on the way is that the default is not test against the random matrix, even though it says:
Test against the small network example. Default: False. I changed that by changing it toaction='store_false', I hope that is how it was originally meant.
@LuzieH about the default setting of --small-network: To have default False one has to have the action 'store_true'. I changed it again. Can you test that works fine for you?
@LuzieH about the default setting of --small-network: To have default False one has to have the action 'store_true'. I changed it again. Can you test that works fine for you?
It works!
@eborrell my update of today:
- I added the validation.py file to src/pytpt (if you dont like it, I can undo it) - in that way we can now check that the inputted transition matrix in the stationary case is irreducible and row-stochastic.
- also have a look at my comment on your commit "vectorization of backward_transitions for the stationary case"
- I split the computation of q_f and q_b in the stationary class and in the periodic case - do we also want it for finite-time? then we have an additional time loop!
Implementing the methods for the case of sparse transition matrices we should really keep for the future (until we need it).
Hey @LuzieH, this is a brief review of whats new:
- the attributes are changed from protected to public. To do so I need to rename the attribute reac_norm_factor.
- the broadcasting tests are done for the periodic case.
- short flags by setting the test parameters (-S, -M and -N) are added.
- I added the methods backward_transitions, forward committor and backward committor for the finite case although I would keep using the method committors because we just need one loop. With the same idea I open a small issue #42 where we can discuss how to optimize the computation of the statistics for the finite case. My idea is to just use to 2 loops over n; 1 for the committors and 1 for the reac density and the reac_current.