husseinaluie / FlowSieve

FlowSieve coarse-graining code base
https://flowsieve.readthedocs.io/en/latest/
Other
18 stars 9 forks source link

Automated tests #13

Closed NoraLoose closed 1 year ago

NoraLoose commented 1 year ago

I see that the package contains tests (in Tests/). I have a few questions:

This issue is part of the JOSS review process over at https://github.com/openjournals/joss-reviews/issues/4277.

bastorer commented 1 year ago

The tests aren't automated. They were originally built during the earlier development phase where we were testing several components. Unfortunately they've been rather neglected lately, and many likely don't work at the moment [although modifying them to use the new function shouldn't be too difficult. I'll add it to the to-do list]. I don't know how to automate the tests, but is something that I'll look into.

The Tutorials are the closest thing that we have for users to test that their FlowSieve installation is up and running.

NoraLoose commented 1 year ago

I think it will be worth resurrecting the testing framework! And it's not just me who is a big fan of tests - JOSS also requires us reviewers to check the following box:

Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?

bastorer commented 1 year ago

Re: tutorials

I'm just about done updating the tutorials. There's now four in total. In addition to SLURM submit scripts, I also include regular bash scripts so that the user can just run the tutorial directly. Except for the largest one, they all run in under 5 minutes on one processor.

  1. "Basic" just a Cartesian doubly-periodic box. Runs in ~1 minute on one processor.
  2. "Scalar" working on a sphere, filters an artificial 'density' field at a couple filter scales. Runs in ~1 min on one processor
  3. "Low Resolution Helmholtz" is essentially a lower resolution version of the old Helmholtz tutorial. Runs in ~5 minutes on one processor
  4. "Helmholtz" is the previous Helmholtz tutorial. On one processor, the three parts take a couple hours to run. With a couple processors in OpenMP ( there's a line in the bash and SLURM scripts to set the number of processors ), it would be 30min - 1 hour. It doesn't show particularly new functionality, so the other tutorials illustrate usage, but gives a clearer sense of what a "production level" run might look like.

I've added Jupyter notebooks for analyzing the results for most of them, just finishing up the last one now (will finish some time tomorrow).

Re: testing

I have some ideas for resurrecting the testing framework, but building up a proper framework is going to take a decent bit of time. With the tutorial Jupyter notebooks including the figures from when I generated them, would those qualify as "manual steps described so that the functionality of the software can be verified?"

Have a nice weekend!

NoraLoose commented 1 year ago

Re: tutorials

Great, thanks for updating the tutorials! I think users will be really grateful for those.

Re: testing

I have some ideas for resurrecting the testing framework, but building up a proper framework is going to take a decent bit of time. With the tutorial Jupyter notebooks including the figures from when I generated them, would those qualify as "manual steps described so that the functionality of the software can be verified?"

I am curious what our editor @kthyng thinks. How strict is JOSS about having a proper testing framework, Kristen?

kthyng commented 1 year ago

From reading the conversation, it sounds like the tutorials will do the job of covering testing in the sense of a user being able to verify the software is working correctly — but @bastorer please make it clear how to run them and that that is how a user can verify they have everything installed properly. Are you saying that the "Tests" directory doesn't work currently? If so would be good to have a readme in there so people don't try to use them, or retire them for now or something.

bastorer commented 1 year ago

That's correct, "Tests" is mostly non-functional at the moment. Rebuilding it into a proper testing framework is on the to-do list, and I have some specific tests in mind, but it's unfortunately necessarily a lower priority on the list right now.

I'll add in a readme right now to avoid confusion, and will transfer them into a new Testing-development branch after taking stock of what all is there / what state they're in.

I'll also add a note near the top of the ReadTheDocs landing page that mentions that the tutorials provide a way to verify that the code is working as expected.

kthyng commented 1 year ago

@NoraLoose I think this covers the JOSS requirement, but please let me know if you have any questions or concerns.

NoraLoose commented 1 year ago

Thanks @kthyng!

Sounds good @bastorer!

NoraLoose commented 1 year ago

I also suggest to remove the "Unit testing" section from the documentation, as long as the tests are not functional: https://flowsieve.readthedocs.io/en/latest/unittests1.html#autotoc_md74

This could otherwise lead to confusion.

bastorer commented 1 year ago

I've added a warning statement to the top of the Unit Testing section, but if you think the full removing it would be better for now, I'm also happy to do that.

NoraLoose commented 1 year ago

Ok, I think this issue can be closed. Thanks for making those changes!