Reuse npse_trained_model fixure for test_npse_map#1461
Reuse npse_trained_model fixure for test_npse_map#1461vagechirkov wants to merge 65 commits intosbi-dev:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1461 +/- ##
==========================================
- Coverage 87.77% 84.82% -2.96%
==========================================
Files 134 137 +3
Lines 11126 11473 +347
==========================================
- Hits 9766 9732 -34
- Misses 1360 1741 +381
Flags with carried forward coverage won't be shown. Click here to find out more. |
janfb
left a comment
There was a problem hiding this comment.
Looks great, thanks!
See question in the comments.
What is the speed up you notice through this change?
|
CI is failing due to |
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. |
|
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 |
|
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
left a comment
There was a problem hiding this comment.
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
|
An issue with snapshot tests: syrupy-project/syrupy#438 |
Handle combinations
…tion for pytest fixtures
| # ToDO check if there is a bug for prior_type='uniform' and num_trial>1 | ||
| # ToDO validate bug for combination sde_type='ode' and num_trial>1 | ||
| # ToDO investigate non-determinism for prior_type=None, dim>= 2, ve, auto/jac_gauss |
There was a problem hiding this comment.
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.
* extract map test
janfb
left a comment
There was a problem hiding this comment.
"Gut Ding will Weile haben".
Apologies @vagechirkov and @schroedk for taking about 6 months to add a review to this PR, but here we go: the general idea and execution is great! You added core fixtures and organize the numerous test case combinations for NPSE in dataclasses.
Unfortunately, we had a huge refactoring in the NPSE and FMPE classes. They have now been unified into the vector field estimator API. The new test file that covers both methods in the old way of testing is linear_gaussian_vector_field_test.py.
I will merge the current main into your branch and fix the conflicts. From there we will need to basically rewrite your approach for the new test file that now covers both NPSE and FMPE. I am happy to help with this.
We can also have a call @schroedk or @vagechirkov or both.
|
@janfb, great! I am happy to help finish implementing this issue! I'm down to have a quick meeting about this 🙂 |
Awesome, let's aim for a meeting this week. How about some time Thu after 10:30? or Friday before noon? |
Friday before noon works for me 🙂 |
Cool! I reached out on Discord and suggested 9am. |
|
@vagechirkov I now managed to push the merge of Feel free to undo the changes in the recent commit, some of them are obsolete. |
|
I copied the regression tests to |
|
Sounds great 🚀 let me know if you need any input from my side. |

#1428