-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add pytests to position #966
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CBroz1
commented
May 8, 2024
CBroz1
commented
May 21, 2024
edeno
reviewed
May 25, 2024
edeno
approved these changes
May 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR increases coverage of the position pipeline to ~72% when run locally.
Full coverage info
Package Changes
I made an effort to avoid changes to
src
wherever possible. Some exceptions include...common_ephys
: fix typocommon_position.PositionVideo.make
: Insert into self at the end of the make func to keep track of processed keys.common_position.PositionVideo.convert_to_pixels
andposition.v1.position_trodes_position
: intest_mode
, truncate the data to avoid mismatches in length for example datacommon_position.PositionVideo.convert_to_pixels
: wrap window management callcv2.DestroyAllWindows
in a try/except to handle an error hit in headless testingdecoding/X/dj_decoder_conversion.py
: When using package scanner tools likeinspect
for Remove unused tables #976 and Merge duplicate funcs #977, I hit issues because of undefined classes in static analysis. Within the try/exceptImportError
, I provide default values for various classes.position/v1/dlc_reader.py
: In test_mode, I skip asserts that would hit on debugging. I think this class could use an overhaul to absorb other duplicative code in the pipeline, and I didn't think it worthwhile to write file-management tools for code I plan to remove laterposition/v1/dlc_utils.py
: Fix issues with my own validator.position.v1.position_dlc_centroid.py
: Add notes where input validation is duplicated, add KeyError whenparams
arg is missing required keysposition.v1.position_dlc_cohort.py
: raise error when assembled table is empty, avoiding failed insertposition.v1.position_dlc_pose_estimation.py
:input
from user, facilitating testingget_video_path
returnsNone
position.v1.position_dlc_position.py
position.v1.position_dlc_selection.py
:skip_dupe
arg oninsert default
, which already handles this caseself.insert
toDLCPosVideo
table to avoid reprocess duplicate keysActions changes
workflow_dispatch
to both publish docks and tests. This will remove the need to push a dummy alpha tag just to publish docs. This also allows a contributor to run the tests on their branch before opening the prconcurrency
is a new feature that replaces the need for our previous 'Cancel workflow actiontest-conda
:matrix
- there is evidence of a previous draft usingmatrix
to iterate over different OS, but we were not using it, and this added more clicking to get into the actual test. If we choose to revert to matrix-ed tests over py version, for example, it's worth the time saving of adding the data files as artifacts across testsenvironment_dlc.yml
. They allow testing a subset of features withinposition
that are not dependent on DeepLabCut, like making output videostest-package-build
Supporting files
CODE_OF_CONDUCT
: linter flagged a 'misspelling', so my markdown linter kicked indocs/misc/mixin
: linter flagged misspellings, added previously missing 'how to add mixin' exampleenvironment
- to reduce installation time in CI/CD, I ...pyproject.toml
pyproject.toml
Tests
README
add notes on environment variables that facilitate running new tests of dlc pipeline headlessly, and other options added to support running non-dlc tests in actions such as--no-docker
and--no-dlc
tests/common
were edited to make use of fixtures migrated to the mainconftest
file, shared across subpackage testsDataDownloader
- new class for managing data downloads. This is only used in local runs for managing the various items pulled from Boxtests/utils
- edits made to accommodate new data population to found in merge chains, and new--no-docker
means of testing role addsChecklist:
CITATION.cff
alter
snippet for release notes.CHANGELOG.md
with PR number and description.