-
Notifications
You must be signed in to change notification settings - Fork 13
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
Reorganise test fixtures #380
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #380 +/- ##
=======================================
Coverage 99.80% 99.80%
=======================================
Files 15 15
Lines 1048 1048
=======================================
Hits 1046 1046
Misses 2 2 ☔ View full report in Codecov by Sentry. |
cc622b9
to
978c8b3
Compare
7c0b12c
to
7acf596
Compare
60b3e50
to
253a8ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @lochhh it looks fantastic!
I really like the consistency (such as the _file
endings, the via_
prefixes, with_nan
suffixes etc), it makes a lot of it much more readable. I followed some of these ideas and suggested a few invalid_
prefixes for some dataset-related fixtures.
The only larger thing is that I had a go at moving the movement_dataset_asserts
fixture to be under the helpers fixtures, since the class that wrapped it only had that one method - it's here, but you can revert if you disagree.
Re PR #385, since that is now merged, if you want to deal with that in this PR I am happy to review any additions. Otherwise we can deal with it separately in another PR/issue.
Oops sorry, we merged it without looking here. |
Co-authored-by: sfmig <[email protected]>
Co-authored-by: sfmig <[email protected]>
Co-authored-by: sfmig <[email protected]>
cc5c7f9
to
8bd6ef1
Compare
|
Thanks for the thorough review and suggestions @sfmig ! I've adopted most suggestions and explained those that weren't. I also added your suggestion related to the renamed I briefly looked into PR #385 and there isn't any new additions to |
Description
What is this PR
Why is this PR needed?
This PR closes #336 and #222.
What does this PR do?
This PR
datasets.py
,files.py
, orhelpers.py
intests/fixtures/
; where datasets contains fixtures for valid and invalid movement datasets and arrays, dataframes; helpers contain the helper functions to count nans and the reusedassert_valid_dataset()
file
fixtures returning file paths are suffixed with "_file" to enable devs to easily identify file fixtures and search for existing onesvia_
valid_poses_dataset
,valid_poses_dataset_with_nan
,valid_position_array
fixtures with the neweruniform_linear_motion
fixtures and updates the affected testsposes_with_nan
fixture to match that of bboxes (i.e. both centroids have nans at the same time points)data
fixtures containing nans are consistently suffixed with_with_nan
(before we had a mix of_with_nans
and_with_nan
)TestComputeKinematics
,TestFilteringValidDataset
id_0
(before, poses ids start fromid_1
); semi-related note: movement by default sets "individuals" names to begin with "individual_0" if none are provided upon import.compute_time_derivative
test to match new function name and parametrise expectationsI haven't changed the vector-related tests in
test_kinematics.py
as there is ongoing work in PR #385.References
#336 #222
How has this PR been tested?
Tests passing
Checklist: