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

Re-organisation/tidying of functional tests submodule #1386

Merged

Conversation

MichaelClerx
Copy link
Member

@MichaelClerx MichaelClerx commented Aug 11, 2021

Hi all,
This is a proposal for a slight re-organisation of the functional tests.

Main changes:

  1. All functional tests have names that match the method they test
  2. Problems are all imported into functionaltests, so you can do e.g. import pints.functionaltests.ft, ft.TestMethodOn...
  3. Functional tests have lost their prefix, so you can write e.g. ft.dream_mcmc.two_dim_gaussian()
  4. Added docstrings everywhere
  5. Added a check to ensure the default warm-up isn't longer than the number of iterations
  6. Tests are explicitly listed for each method (instead of using a name-matching system).

Test discovery example:

import pints
import pints.functionaltests as ft

print('All tests')
for x in ft.tests():
    print(x)

print()
print('All tests for population MCMC')
for x in ft.tests(pints.PopulationMCMC):
    print(x)

@MichaelClerx MichaelClerx marked this pull request as ready for review August 11, 2021 12:14
@MichaelClerx MichaelClerx requested review from ben18785, fcooper8472, chonlei, DavAug, FarmHJ and rccreswell and removed request for FarmHJ August 11, 2021 12:14
@MichaelClerx
Copy link
Member Author

MichaelClerx commented Aug 11, 2021

Hi functional testers @ben18785 @fcooper8472 @chonlei @DavAug @rcw5890 and anyone I'm forgetting,

I've done a proposal to make the stuff we've been adding here a bit more streamlined and usable. Changes are listed above.

Due to a naming change, the diff shows up as old fine gone, new file added, sorry about that.

Please let me know what you think!

@MichaelClerx
Copy link
Member Author

I think this is still all in line with @fcooper8472 's original design: pints-team/change-point-testing#38

Although I think the updated plan was to have functional tests live somewhere outside PINTS.

One solution would be to have:

  • pints
    • code, docs, unit tests, examples, etc
  • pints method-merge-tests
  • pints functional testing
    • imports pints and pints method-merge-tests
    • runs the tests that are currently under pints.functionaltests, stores the results, etc.

That fits with the idea that you make the pre-merge test first, then use that to create the functional test, and then re-use it if the functional test ever fails.

Not sure how the method-merge-tests get access to unmerged new methods though (that's a problem with the current set-up too)?

@fcooper8472
Copy link
Member

If there was an updated plan to have the functional testing methods themselves outside PINTS then I don't think I was involved in that conversation. The existing structure was the result of the most recent decision that I'm aware of.

The changes in this PR look fine to me, although I haven't looked at any of the changes in individual files because, as you say, the diffs are all obscured due to renaming.

@MichaelClerx
Copy link
Member Author

MichaelClerx commented Aug 11, 2021

Thanks!

If there was an updated plan to have the functional testing methods themselves outside PINTS then I don't think I was involved in that conversation. The existing structure was the result of the most recent decision that I'm aware of.

I just meant your updated ideas here: pints-team/change-point-testing#38 (comment)

Although reading that more carefully I'm not sure what you mean there 😅

I'm happy to keep the functional tests in PINTS!

@fcooper8472
Copy link
Member

I think the plan was to have the methods themselves in PINTS, but the script that handles the automated running of those methods and the storing/processing of the data (the "functional testing" infrastructure) would live in another repo.

@MichaelClerx
Copy link
Member Author

Gotcha!

If we're doing that, I wouldn't mind having the method merge tests in the pints repo too, if (big if!) we're OK with them not being tested and using dependencies not required by pints.

@MichaelClerx
Copy link
Member Author

@ben18785 can you have a quick look at this one? Hoping to merge this soon so that it doesn't keep going out of date

Copy link
Collaborator

@ben18785 ben18785 left a comment

Choose a reason for hiding this comment

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

Thanks @MichaelClerx -- I've been through and checked things (mostly that the number of iterations, warmup and chains match) and looks good. Given that these things are part of PINTS, how should we handle the Jupyter notebooks in the method-merge-tests repo? Do they need a particular PINTS commit associated with them somewhere?

@MichaelClerx
Copy link
Member Author

Thanks @MichaelClerx -- I've been through and checked things (mostly that the number of iterations, warmup and chains match) and looks good. Given that these things are part of PINTS, how should we handle the Jupyter notebooks in the method-merge-tests repo? Do they need a particular PINTS commit associated with them somewhere?

Thanks @ben18785 !

I think we can just update those whenever we need to run them? There shouldn't be many (further) updates to this code anyway, I think

@MichaelClerx
Copy link
Member Author

I'll merge this in tomorrow and update the method merge tests

@MichaelClerx MichaelClerx merged commit e89e2ba into issue-1294-functional-testing-module Aug 17, 2021
@MichaelClerx MichaelClerx deleted the func-test-organising branch August 17, 2021 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants