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

Test improvements today #334

Merged
merged 7 commits into from
May 26, 2023
Merged

Test improvements today #334

merged 7 commits into from
May 26, 2023

Conversation

Jacob-Stevens-Haas
Copy link
Member

@Jacob-Stevens-Haas Jacob-Stevens-Haas commented May 12, 2023

Beginning to work on #320. A variety of changes to speed up testing, from around 370 to 154 seconds

  • Dropped testing notebook 5 and 17. 5 will become a lot faster once we move fast FiniteDifference into derivative, since most of the notebook runtime was doing that comparison; then we can test it again
  • Removed unneccessary parameterization, i.e. in testing ensembling, no need to test every inner optimizer because they are independent. Just use the fastest.
  • flattened test directory, e.g. test/optimizers/test_optimizers.py becomes test/test_optimizers.py
  • Shortened the most commonly used test data (data_lorenz from 500 data points to 50)
  • That broke a few things, which provided an opportunity to decouple testing that didn't need to be coupled, and also to make implicit coupling explicit.

I'll continue with in a new PR next week or after a short break. The following test accounts for one third of the remaining test time:

28.81s call     test/test_sindyc.py::test_simulate_with_interp[data_lorenz_c_2d]
28.28s call     test/test_sindyc.py::test_simulate_with_interp[data_lorenz_c_1d]

5 and 17 are culprits.  After the fast FiniteDifference is moved from pysindy
to derivative, 5 will be faster.

Also gitignore output files from 17.
We don't need to test legacy ensembling behavior for every optimizer,
since the ensembling part doesn't depend upon the inner optimizer.  We also
don't need 10x10 bagging x library_bagging, we just need 2x2 to test
functionality.

Old test durations:
4.82s call     test/optimizers/test_optimizers.py::test_legacy_ensemble_odes[StableLinearSR3]
1.69s call     test/optimizers/test_optimizers.py::test_legacy_ensemble_odes[MIOSR]
0.36s call     test/optimizers/test_optimizers.py::test_legacy_ensemble_odes[TrappingSR3]
0.34s call     test/optimizers/test_optimizers.py::test_legacy_ensemble_odes[FROLS]
0.11s call     test/optimizers/test_optimizers.py::test_legacy_ensemble_odes[SSR]
0.04s call     test/optimizers/test_optimizers.py::test_legacy_ensemble_odes[ConstrainedSR3]
0.04s call     test/optimizers/test_optimizers.py::test_legacy_ensemble_odes[SR3]
0.04s call     test/optimizers/test_optimizers.py::test_legacy_ensemble_odes[STLSQ]

New test duration:
0.02s call     test/optimizers/test_optimizers.py::test_legacy_ensemble_odes
Also, make test/differentiation/test_differentiation_methods.py just
test/test_differentiation.py.  Unneccessary nesting.

Previously, test_nan_derivatives asked whether the model fit by SINDy when
FiniteDifference gave NaN endpointswas close to the model fit without nans.
This is an integration test that is really asking whether FiniteDifference is
calculating derivatives in a consistent with drop_endpoints=True and False.

Also, a bit faster but this test was fast to begin with.
Previously, test_complexity and test_coefficients just tested that
- required names are present in SINDy object
- when fit on some amount of data, less than 10 terms were nonzero
- These are essentially the same test, since model.complexity is just
  a wrapper around it's optimizer's complexity property

But Lorenz has 7 nonzero terms, and when I changed the data to speed up
tests, data had a few more nonzeros.
We don't need 500 timepoints; shortening to 50.

Also, data_sindypi_library and data_pde_library had an implicit
dependence upon data_lorenz; this commit makes that dependence explicit.
A lot of test files were nested under a directory with just one file.  It's now
one less click to view test results or files.
@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -0.41 ⚠️

Comparison is base (44f04ab) 92.31% compared to head (c9286d8) 91.91%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
- Coverage   92.31%   91.91%   -0.41%     
==========================================
  Files          37       37              
  Lines        3747     3747              
==========================================
- Hits         3459     3444      -15     
- Misses        288      303      +15     

see 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@znicolaou
Copy link
Collaborator

@Jacob-Stevens-Haas hoping to find some time to review this and the other PRs in the next week or two!

@Jacob-Stevens-Haas
Copy link
Member Author

It looks like what test_simulate_with_interp is testing is specifically simulate with a Callable passed as the control input, rather than specifically the callable from interp1d. Is that correct?

If that's the case, rather than simulating Lorenz, training a model, and creating an interp function, I'll just mock up a model with given coefficients, maybe choose simpler dynamics, and pass a fast callable. Looks like cutting the number of integrations in half, a simpler ODE to integrate, and a reduced number of time steps down

Profiling results
   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        2    0.000    0.000   91.496   45.748 runner.py:111(pytest_runtest_protocol)
        2    0.000    0.000   91.494   45.747 runner.py:119(runtestprotocol)
        6    0.000    0.000   91.494   15.249 runner.py:219(call_and_report)
    38/22    0.000    0.000   91.493    4.159 _hooks.py:244(__call__)
    38/22    0.000    0.000   91.493    4.159 _manager.py:77(_hookexec)
    38/22    0.001    0.000   91.493    4.159 _callers.py:9(_multicall)
        6    0.000    0.000   91.491   15.249 runner.py:247(call_runtest_hook)
        6    0.000    0.000   91.491   15.248 runner.py:318(from_call)
        6    0.000    0.000   91.491   15.248 runner.py:262(<lambda>)
        4    0.156    0.039   91.446   22.861 ivp.py:156(solve_ivp)
        2    0.000    0.000   91.420   45.710 runner.py:160(pytest_runtest_call)
        2    0.000    0.000   91.420   45.710 python.py:1797(runtest)
        2    0.000    0.000   91.419   45.710 python.py:187(pytest_pyfunc_call)
        2    0.000    0.000   91.419   45.710 test_sindyc.py:191(test_simulate_with_interp)
        2    0.000    0.000   91.382   45.691 pysindy.py:740(simulate)
    25052    0.079    0.000   90.927    0.004 base.py:159(step)
    24917    0.123    0.000   90.800    0.004 lsoda.py:144(_step_impl)
    24917    0.616    0.000   90.660    0.004 _ode.py:1336(run)
    55697    0.097    0.000   90.054    0.002 base.py:136(fun)
    55697    0.085    0.000   89.956    0.002 base.py:19(fun_wrapped)

@Jacob-Stevens-Haas
Copy link
Member Author

I'm going to go ahead an merge this PR so that I can troubleshoot the codecov issue, which is a lot easier with faster tests. If anyone needs me to undo it, I can.

@Jacob-Stevens-Haas Jacob-Stevens-Haas merged commit b2d83f8 into master May 26, 2023
@Jacob-Stevens-Haas Jacob-Stevens-Haas deleted the simpler-tests branch May 26, 2023 18:14
@znicolaou
Copy link
Collaborator

Thumbs up! FYI, I am going to push an update to notebook 17 in the next few days as I finalize some revisions for a resubmission. I made some minor changes to allow the notebook test to end early so that I don't have to construct a huge number of dummy data sets for all the various tests, which I think is the best solution to make the test reasonable. You also asked about the plt.ion in the other pull request a while back -- I had included that to stop the test from hanging with plt.show() on my system until the plot is closed, but feel free to remove if it works without it.

jpcurbelo pushed a commit to jpcurbelo/pysindy_fork that referenced this pull request Apr 30, 2024
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