mandate tests for additions to MDAnalysis.analysis and introduction of analysis.legacy

[orbeckst]

tl;dr: Spring cleaning for MDAnalysis.analysis.

With recent discussions about the analysis module (#666, #719) and the feeling that we need to get it up to the same standards as the core, I think an important step is better tests and identifying to the user code that is not well maintained (it's sad having to admit that we have under-maintained code but this is a fact in open source and we simply have a responsibility towards our users).

• Mandatory tests for new analysis code: P1
• MDAnalysis.analysis.legacy module for unmaintained code: P2

What say ye, @MDAnalysis/coredevs ?

(P1) Mandatory tests for new analysis code

I propose to require new code for MDAnalysis.analysis to come with unit tests. Currently, the Style Guide only encourages unit tests.

Tests for analysis classes and functions should at a minimum perform regression tests, i.e., run on input and compare to values generated when when the code was added so that we know when the output changes in the future. (Even better are tests that test for absolute correctness of results but regression tests are the minimum requirement.)

If we adopt (P1), the Style Guide will be changed to reflect this policy and new analysis code will not be merged until tests are provided and pass.

(P2) MDAnalysis.analysis.legacy for unmaintained code

I am also proposing that any code in MDAnalysis.analysis that does not have substantial testing (at least 70% coverage... we can debate this number!) will be moved to a special MDAnalysis.analysis.legacy module that will come with its own warning that this is essentially unmaintained functionality that is still provided because there's no alternative. Legacy packages that receive sufficient upgrades can come back to the normal MDAnalysis.analysis name space.

If we adopt P2 then modules can be moved to legacy as soon as we decide on its creation but until release 1.0 we leave stubs in place so that no old code breaks (and issue deprecation warnings for the import). Once 1.0 is released, the stubs will be removed and the modules will only be available from legacy.

History

• 2016-02-26 initial proposal
• 2016-03-12 added @richardjgowers 's suggestion to require regression tests at a minimum
• 2016-03-24 P1 and P2 accepted

[richardjgowers]

I like this, with the twist that we should just require a regression test on each function/method.

I can't remember who said this (@dotsdl probably?), but we could(?) move analysis/vis into their own repo, but still package it all together as one package (so pip install MDAnalysis gets you everything). Then PRs like #708 #735 could go to the analysis repo, with this repo being the "main" stuff (something like a numpy/scipy split). Then we could monitor coverage of analysis+vis separately to core.

[tylerjereddy]

Sounds sensible. I guess my MDAnalysis.visualization stuff won't be subject
to this just yet though. I think a bit of creativity would be needed to get
sensible tests in there.

On 27 February 2016 at 11:32, Richard Gowers notifications@github.com
wrote:

I like this, with the twist that we should just require a regression test
on each function/method.

I can't remember who said this (@dotsdl https://github.com/dotsdl
probably?), but we could(?) move analysis/vis into their own repo, but
still package it all together as one package (so pip install MDAnalysis
gets you everything). Then PRs like #708
#708 #735
#735 could go to the
analysis repo, with this repo being the "main" stuff (something like a
numpy/scipy split). Then we could monitor coverage of analys is+vis
separately to core.

—
Reply to this email directly or view it on GitHub
#743 (comment)
.

[richardjgowers]

It might be possible just to assert that a matplotlib object gets returned
for analysis.... So at least the code didn't explode. Beyond that it's
tricky to inspect images.

On Wed, 9 Mar 2016 15:57 Tyler Reddy, notifications@github.com wrote:

Sounds sensible. I guess my MDAnalysis.visualization stuff won't be subject
to this just yet though. I think a bit of creativity would be needed to get
sensible tests in there.

On 27 February 2016 at 11:32, Richard Gowers notifications@github.com
wrote:

I like this, with the twist that we should just require a regression test
on each function/method.

I can't remember who said this (@dotsdl https://github.com/dotsdl
probably?), but we could(?) move analysis/vis into their own repo, but
still package it all together as one package (so pip install MDAnalysis
gets you everything). Then PRs like #708
#708 #735
#735 could go to the
analysis repo, with this repo being the "main" stuff (something like a
numpy/scipy split). Then we could monitor coverage of analys is+vis
separately to core.

—
Reply to this email directly or view it on GitHub
<
#743 (comment)

.

—
Reply to this email directly or view it on GitHub
#743 (comment)
.

[kain88-de]

prettyplotlib compares to some baseline images to test the plotting routines. But other then that I'm also not sure what to do to test images.

[tylerjereddy]

That's true. Although the streamline code does do some mathematics before producing the image so I could at least provide tests for that, or the data before it is plotted. I think any tests would likely just prevent regression / changing of behaviour rather than proving that the code is correct, but that is likely better than the current state (no tests). I'll make a note of this...

[dotsdl]

Since matplotlib figures are just python objects (they're only made into images when you do Figure.savefig or use one of the backends to display them), one could just inspect the Figure and its Axes objects to make sure everything looks right. Everything is in the Figure object, basically.

[orbeckst]

Added @richardjgowers 's suggestion to require regression tests (at a minimum).

[orbeckst]

Quick comments on the discussion:

• Maybe we open a separate proposal for MDAnalysis.visualization --- I skirted this issue here and focused on MDAnalysis.analysis.
• Let's discuss the feasibility of splitting the library in a different issue. Right now this sounds like opening up yet another big construction site... let's get Move from Atom based topology to array based #363 done before starting anything else of similar magnitude.

Do we have any other voices? Or can we consider this proposal, consisting of P1 and P2, consider accepted? (GitHub does not have proper voting but you can use the thumbs up/down emoticons on the top comment.)

[kain88-de]

I'm for both solutions. @orbeckst should we just count the like to your post that people agree with P1 and P2