Reuse npse_trained_model fixure for test_npse_map

[vagechirkov]

#1428

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.70%. Comparing base (a1d7555) to head (7276ad4).
Report is 16 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1461       +/-   ##
===========================================
- Coverage   89.60%   78.70%   -10.90%     
===========================================
  Files         121      127        +6     
  Lines        9357    10025      +668     
===========================================
- Hits         8384     7890      -494     
- Misses        973     2135     +1162     

Flag	Coverage Δ
unittests	78.70% <ø> (-10.90%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 58 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[janfb]

Looks great, thanks!
See question in the comments.

What is the speed up you notice through this change?

[janfb]

CI is failing due to testmon error. this will be fixed in a separate PR

[vagechirkov]

What is the speed up you notice through this change?

Basically, we save the time of training a model specifically for `test_npse_map'. But now it would be much cheaper to add more tests that can reuse specific trained models.

[janfb]

One concern I have @vagechirkov is that now all the tests here are marked as slow, right?

Is there a way to have one of the c2st tests "not slow"? e.g., the one with the default settings we use in the class?

[vagechirkov]

Hey @janfb, I think this is more or less how the final test structure should look like, so ready for review 🙃

[janfb]

Hey @janfb, I think this is more or less how the final test structure should look like, so ready for review 🙃

Cool, I will review it now.

[janfb]

thanks a lot @schroedk and @vagechirkov ! 👏

it all looks very clean and efficient now. Added a couple of questions for my understanding and some suggestions

[vagechirkov]

An issue with snapshot tests: syrupy-project/syrupy#438

[schroedk]

@vagechirkov @janfb my suggestion for this PR would be:

• revert to the previous version of the test file linearGaussian_npse_test.py
• add pytest.mark.slow to the tests
• add the regression test (rename from test_snapshot -> linearGaussian_npse_test_regression.py)
• remove the ambr file (and also clean the history, as I forgot to put it into lfs, my bad:(...)

This would not show the refactoring of the test (which we aimed for at the beginning), but at least it shows the usage of pytest-regression. What do you think?

[vagechirkov]

Alright, I think I’ve finally tracked down the issue 🎉🫠 (was seriously starting to question my sanity). Turns out it’s all about the combination of parameters — nothing to do with how we wrote the tests 🤪 I used the pytest-timeout plugin with @pytest.mark.timeout(60) to catch it. These are the tests that take longer than 60 seconds to complete:

• uniform_prior-dim_3-ve-fnpe-sde-trials_8
• uniform_prior-dim_3-ve-auto_gauss-sde-trials_8
• uniform_prior-dim_3-ve-jac_gauss-sde-trials_8
• uniform_prior-dim_3-vp-fnpe-sde-trials_8
• uniform_prior-dim_3-subvp-fnpe-sde-trials_8

This means, in turn, that this seems to be a separate issue. What do you think, @janfb?

P.S.
The reason we didn’t hit this issue in test_spnapshot.py is because there’s actually a bug there — we’re not really using the number of trials 🙈 @schroedk

[vagechirkov]

Here is the longer list of slow regression tests:

[vagechirkov]

Very nice refactoring, thanks @schroedk! Regression tests run locally for me in 4min without the problem reported here: #1473

[vagechirkov]

I was wondering if we should mark these tests (non-deterministic and those that take too long) as xfail and set the timeout to, say, 20 seconds. This way, we would cover all possible combinations with the regression tests, and failing tests could be resolved later in the separate PRs.