Allowing inference on multiple videos via sleap-track
#1784
Conversation
WalkthroughThe recent updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/nn/inference.py (4 hunks)
Additional Context Used
Ruff (3)
sleap/nn/inference.py (3)
149-149: Undefined name
Task
584-584: Local variable
outputsis assigned to but never used
4615-4615: Do not compare types, use
isinstance()
Additional comments not posted (2)
sleap/nn/inference.py (2)
5291-5340: Refactor to handle multiple data paths and providers looks good. Consider adding comments to explain the handling of different file types.
5474-5474: Integration of the new provider and data path handling in data loader setup looks correct. Ensure compatibility with the tracker setup.Verification successful
The
_make_tracker_from_clifunction and its usage appear to be correctly integrated with the new data structures. The function creates a tracker based on CLI arguments, and the test file verifies its output, ensuring compatibility.
sleap/nn/inference.py:
- Function definition:
_make_tracker_from_cli(args: argparse.Namespace) -> Optional[Tracker]- The function creates a tracker based on parsed CLI arguments.
tests/nn/test_inference.py:
- The function is used to create a tracker and verify its output.
The integration of the new provider and data path handling in the data loader setup looks correct. Ensure compatibility with the tracker setup.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify tracker setup compatibility with new data structures. # Test: Search for the tracker setup function and its usage. Expect: Compatibility with new data structures. rg --type python $'_make_tracker_from_cli'Length of output: 72
Script:
#!/bin/bash # Description: Verify tracker setup compatibility with new data structures. # Test: Search for the tracker setup function and its usage. Expect: Compatibility with new data structures. rg --type py '_make_tracker_from_cli'Length of output: 328
Script:
#!/bin/bash # Description: Extract the `_make_tracker_from_cli` function definition and its usage. # Extract the function definition. rg --type py --context 10 'def _make_tracker_from_cli' sleap/nn/inference.py # Extract the usage in the test file. rg --type py --context 10 '_make_tracker_from_cli' tests/nn/test_inference.pyLength of output: 1899
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/nn/inference.py (3 hunks)
Additional Context Used
Ruff (3)
sleap/nn/inference.py (3)
149-149: Undefined name
Task
584-584: Local variable
outputsis assigned to but never used
4615-4615: Do not compare types, use
isinstance()
Additional comments not posted (1)
sleap/nn/inference.py (1)
5600-5600: Refine the error message to provide more context and guidance.- "Use \"sleap-track --tracking.tracker ...' to specify tracker to use." + "Specify the tracker using 'sleap-track --tracking.tracker'. Refer to the documentation for more details."
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/nn/inference.py (4 hunks)
Additional Context Used
Ruff (3)
sleap/nn/inference.py (3)
149-149: Undefined name
Task
584-584: Local variable
outputsis assigned to but never used
4615-4615: Do not compare types, use
isinstance()
Additional comments not posted (1)
sleap/nn/inference.py (1)
5607-5607: The device setup logic is correctly implemented to handle both CPU and GPU based on the user's configuration.
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/nn/inference.py (4 hunks)
Additional Context Used
Ruff (3)
sleap/nn/inference.py (3)
149-149: Undefined name
Task
584-584: Local variable
outputsis assigned to but never used
4615-4615: Do not compare types, use
isinstance()
Additional comments not posted (2)
sleap/nn/inference.py (2)
5611-5611: Refine the error message to provide more context and guidance.
5611-5611: Refine the error message to provide more context and guidance.
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (10)
tests/nn/test_inference.py (10)
Line range hint
11-11: Remove unused import:tensorflow_hub.- import tensorflow_hub as hubTools
Ruff
6-6:
osimported but unused (F401)Remove unused import:
os
Line range hint
15-15: Remove unused import:sleap.gui.learning.runners.- from sleap.gui.learning import runnersTools
Ruff
6-6:
osimported but unused (F401)Remove unused import:
os
Line range hint
23-24: Remove unused imports related toInferenceLayerandInferenceModel.- from sleap.nn.inference import ( - InferenceLayer, - InferenceModel,Tools
Ruff
6-6:
osimported but unused (F401)Remove unused import:
os
Line range hint
57-57: Remove unused import:sleap.nn.tracking.MatchedFrameInstance.- from sleap.nn.tracking import ( - MatchedFrameInstance,Tools
Ruff
6-6:
osimported but unused (F401)Remove unused import:
os
Line range hint
62-62: Remove unused import:sleap.instance.Track.- from sleap.instance import TrackTools
Ruff
6-6:
osimported but unused (F401)Remove unused import:
os
Line range hint
605-605: Replace equality comparisons toFalseandTruewith more Pythonic expressions.- if predictor.is_grayscale == False + if not predictor.is_grayscale - if predictor.is_grayscale == True + if predictor.is_grayscaleAlso applies to: 653-653, 702-702, 735-735, 1644-1644
Tools
Ruff
6-6:
osimported but unused (F401)Remove unused import:
os
Line range hint
890-890: Remove assignment to unused variablepredictor.- predictor = load_model(model_path)Tools
Ruff
6-6:
osimported but unused (F401)Remove unused import:
os
Line range hint
1249-1249: Consider replacingpytest.raises(Exception)with a more specific exception type to improve test clarity and error handling.Tools
Ruff
6-6:
osimported but unused (F401)Remove unused import:
os
Line range hint
1379-1379: Remove assignment to unused variablelabels.- labels = min_tracks_2node_labelsTools
Ruff
6-6:
osimported but unused (F401)Remove unused import:
os
Line range hint
1565-1565: Usekey in dictinstead ofkey in dict.keys()for more efficient dictionary lookups.- for track in tracker.track_matching_queue_dict.keys(): + for track in tracker.track_matching_queue_dict: - for key in tracker.candidate_maker.shifted_instances.keys(): + for key in tracker.candidate_maker.shifted_instances:Also applies to: 1609-1609, 1614-1614
Tools
Ruff
6-6:
osimported but unused (F401)Remove unused import:
os
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (4 hunks)
- tests/nn/test_inference.py (4 hunks)
Additional context used
Ruff
tests/nn/test_inference.py
6-6:
osimported but unused (F401)Remove unused import:
os
11-11:
tensorflow_hubimported but unused (F401)Remove unused import:
tensorflow_hub
15-15:
sleap.gui.learning.runnersimported but unused (F401)Remove unused import:
sleap.gui.learning.runners
23-23:
sleap.nn.inference.InferenceLayerimported but unused (F401)Remove unused import
24-24:
sleap.nn.inference.InferenceModelimported but unused (F401)Remove unused import
57-57:
sleap.nn.tracking.MatchedFrameInstanceimported but unused (F401)Remove unused import:
sleap.nn.tracking.MatchedFrameInstance
62-62:
sleap.instance.Trackimported but unused (F401)Remove unused import:
sleap.instance.Track
605-605: Avoid equality comparisons to
False; useif not predictor.is_grayscale:for false checks (E712)Replace with
not predictor.is_grayscale
653-653: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
702-702: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
735-735: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
794-794: Pointless comparison. Did you mean to assign a value? Otherwise, prepend
assertor remove it. (B015)
795-795: Pointless comparison. Did you mean to assign a value? Otherwise, prepend
assertor remove it. (B015)
890-890: Local variable
predictoris assigned to but never used (F841)Remove assignment to unused variable
predictor
1249-1249:
pytest.raises(Exception)should be considered evil (B017)
1379-1379: Local variable
labelsis assigned to but never used (F841)Remove assignment to unused variable
labels
1565-1565: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1609-1609: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1614-1614: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1644-1644: Avoid equality comparisons to
False; useif not predictor.is_grayscale:for false checks (E712)Replace with
not predictor.is_grayscalesleap/nn/inference.py
131-131:
typing.Textis deprecated, usestr(UP019)Replace with
str
149-149: Undefined name
Task(F821)
584-584: Local variable
outputsis assigned to but never used (F841)Remove assignment to unused variable
outputs
605-605:
typing.Textis deprecated, usestr(UP019)Replace with
str
613-613:
typing.Textis deprecated, usestr(UP019)Replace with
str
627-627:
typing.Textis deprecated, usestr(UP019)Replace with
str
641-641:
typing.Textis deprecated, usestr(UP019)Replace with
str
695-696: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
1183-1183: No explicit
stacklevelkeyword argument found (B028)
1325-1328: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
1468-1468:
typing.Textis deprecated, usestr(UP019)Replace with
str
1758-1763: Use ternary operator
imgs = inputs["image"] if isinstance(inputs, dict) else inputsinstead ofif-else-block (SIM108)
2394-2394:
typing.Textis deprecated, usestr(UP019)Replace with
str
2395-2395:
typing.Textis deprecated, usestr(UP019)Replace with
str
2822-2825: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
3110-3110:
typing.Textis deprecated, usestr(UP019)Replace with
str
3424-3427: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
3436-3439: Use ternary operator
offsets = preds[self.offsets_ind] if self.offsets_ind is not None else Noneinstead ofif-else-block (SIM108)Replace
if-else-block withoffsets = preds[self.offsets_ind] if self.offsets_ind is not None else None
3654-3654:
typing.Textis deprecated, usestr(UP019)Replace with
str
3756-3756: Loop control variable
imagenot used within loop body (B007)Rename unused
imageto_image
4262-4262:
typing.Textis deprecated, usestr(UP019)Replace with
str
4263-4263:
typing.Textis deprecated, usestr(UP019)Replace with
str
4452-4452: Loop control variable
imagenot used within loop body (B007)Rename unused
imageto_image
4615-4619: Use ternary operator
img = ex["image"] if type(ex) == dict else exinstead ofif-else-block (SIM108)Replace
if-else-block withimg = ex["image"] if type(ex) == dict else ex
4615-4615: Do not compare types, use
isinstance()(E721)
4709-4709:
typing.Textis deprecated, usestr(UP019)Replace with
str
4873-4876: Use ternary operator
model_paths = [model_path] if isinstance(model_path, str) else model_pathinstead ofif-else-block (SIM108)Replace
if-else-block withmodel_paths = [model_path] if isinstance(model_path, str) else model_path
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-block (SIM108)Replace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)
5350-5350: Local variable
eis assigned to but never used (F841)Remove assignment to unused variable
e
Additional comments not posted (7)
tests/nn/test_inference.py (1)
1486-1537: The test functiontest_sleap_track_mult_inputis designed to handle multiple input files for tracking. Ensure that the test covers all necessary assertions and cleanup operations correctly.Verification successful
The test function
test_sleap_track_mult_inputand its related functionstest_sleap_track_mult_inputs_folder_slp_filesandtest_sleap_track_mult_inputs_folderhave been successfully extracted. The implementation shows that the test function is designed to handle multiple input files for tracking and includes assertions to ensure the existence of prediction files and cleanup operations.
test_sleap_track_mult_inputhandles multiple files by iterating over the directory contents and asserting the existence of files before and after running thesleap_trackcommand.- The function
test_sleap_track_mult_inputs_folder_slp_filesreturns a path to a directory containing multiple input files.- The function
test_sleap_track_mult_inputs_folderis defined as a fixture but its implementation is not fully shown.Based on the extracted code, the test function appears to cover the necessary assertions and cleanup operations correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the test function `test_sleap_track_mult_input` handles multiple files correctly. # Test: Search for the function usage. Expect: Proper handling of multiple files. rg --type python $'test_sleap_track_mult_input'Length of output: 77
Script:
#!/bin/bash # Description: Verify that the test function `test_sleap_track_mult_input` handles multiple files correctly. # Test: Search for the function usage. Expect: Proper handling of multiple files. rg --type py $'test_sleap_track_mult_input'Length of output: 578
Script:
#!/bin/bash # Description: Extract the implementation of `test_sleap_track_mult_input` and related functions from `tests/nn/test_inference.py`. # Extract the implementation of the function `test_sleap_track_mult_input` and related functions. ast-grep --lang python --pattern $'def test_sleap_track_mult_input($_$$$) { $$$ }' tests/nn/test_inference.py ast-grep --lang python --pattern $'def test_sleap_track_mult_inputs_folder_slp_files($_$$$) { $$$ }' tests/nn/test_inference.py ast-grep --lang python --pattern $'def test_sleap_track_mult_inputs_folder($_$$$) { $$$ }' tests/nn/test_inference.pyLength of output: 1803
Script:
#!/bin/bash # Description: Extract the implementation of `test_sleap_track_mult_input` and related functions from `tests/nn/test_inference.py`. # Extract the implementation of the function `test_sleap_track_mult_input` and related functions. ast-grep --lang python --pattern 'def test_sleap_track_mult_input($_$$$): $$$' tests/nn/test_inference.py ast-grep --lang python --pattern 'def test_sleap_track_mult_inputs_folder_slp_files($_$$$): $$$' tests/nn/test_inference.py ast-grep --lang python --pattern 'def test_sleap_track_mult_inputs_folder($_$$$): $$$' tests/nn/test_inference.pyLength of output: 349
Script:
#!/bin/bash # Description: Search for the function definitions of `test_sleap_track_mult_input` and related functions in `tests/nn/test_inference.py`. # Search for the function definitions. rg 'def test_sleap_track_mult_input' tests/nn/test_inference.py rg 'def test_sleap_track_mult_inputs_folder_slp_files' tests/nn/test_inference.py rg 'def test_sleap_track_mult_inputs_folder' tests/nn/test_inference.pyLength of output: 501
Script:
#!/bin/bash # Description: Extract the full implementation of `test_sleap_track_mult_input` and related functions from `tests/nn/test_inference.py`. # Extract the full implementation of the function `test_sleap_track_mult_input`. sed -n '/def test_sleap_track_mult_input/,/^def/p' tests/nn/test_inference.py # Extract the full implementation of the function `test_sleap_track_mult_inputs_folder_slp_files`. sed -n '/def test_sleap_track_mult_inputs_folder_slp_files/,/^def/p' tests/nn/test_inference.py # Extract the full implementation of the function `test_sleap_track_mult_inputs_folder`. sed -n '/def test_sleap_track_mult_inputs_folder/,/^def/p' tests/nn/test_inference.pyLength of output: 2565
sleap/nn/inference.py (6)
5300-5307: Ensure proper handling of directories and files. The current implementation is correct but consider adding a comment about the expected directory structure or file types for clarity.
5448-5448: Logging the parsed arguments is good for debugging. Ensure sensitive information is not logged.
5488-5488: Correct setup of data loaders and trackers. Ensure that_make_provider_from_cliand_make_tracker_from_cliare robust and handle errors appropriately.
5510-5564: This block handles the core functionality of running inference on multiple inputs. Ensure that all edge cases are handled, such as empty input lists or errors during processing.
5568-5617: The tracking logic appears robust. Consider adding more detailed logging to help with debugging and ensure that all paths are correctly reset after each iteration.
5627-5627: Refine the error message to provide more context and guidance.
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (12)
tests/nn/test_inference.py (11)
Line range hint
10-10: Remove unused import:tensorflow_hub.- import tensorflow_hub as hub
Line range hint
14-14: Remove unused import:sleap.gui.learning.runners.- from sleap.gui.learning import runners
Line range hint
22-23: Remove unused imports related to inference layers and models.- from sleap.nn.inference import InferenceLayer - from sleap.nn.inference import InferenceModel
Line range hint
56-56: Remove unused import:sleap.nn.tracking.MatchedFrameInstance.- from sleap.nn.tracking import MatchedFrameInstance
Line range hint
61-61: Remove unused import:sleap.instance.Track.- from sleap.instance import Track
Line range hint
608-608: Replace equality comparisons to boolean values with direct truthy or falsy checks.- if predictor.is_grayscale == False + if not predictor.is_grayscale - if predictor.is_grayscale == True + if predictor.is_grayscaleAlso applies to: 656-656, 705-705, 738-738, 1743-1743
Line range hint
797-798: These lines appear to be comparisons without assertions or any other form of logical continuation. If these are intended to be assertions, please add theassertkeyword; otherwise, these lines should be removed or corrected.- out["instance_peaks"].shape == (2, 1, 17, 2) + assert out["instance_peaks"].shape == (2, 1, 17, 2) - out["instance_peak_vals"].shape == (2, 1, 17) + assert out["instance_peak_vals"].shape == (2, 1, 17)
Line range hint
893-893: Remove assignment to unused variablepredictor.- predictor = load_model(model_path)
Line range hint
1252-1252: Usingpytest.raises(Exception)is generally considered too broad. It is better to specify the exact type of exception expected to make the test more precise and informative.- with pytest.raises(Exception): + with pytest.raises(SpecificExceptionType):
Line range hint
1382-1382: Remove assignment to unused variablelabels.- labels: Labels = centered_pair_predictions
Line range hint
1664-1664: Use direct dictionary key checks instead of calling.keys().- if key in dict.keys(): + if key in dict:Also applies to: 1708-1708, 1713-1713
sleap/nn/inference.py (1)
5448-5448: Consider adding more descriptive comments or logging for debugging purposes.It might be helpful to add more context to the printed arguments for easier debugging and maintenance.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (4 hunks)
- tests/nn/test_inference.py (3 hunks)
Additional context used
Ruff
tests/nn/test_inference.py
10-10:
tensorflow_hubimported but unused (F401)Remove unused import:
tensorflow_hub
14-14:
sleap.gui.learning.runnersimported but unused (F401)Remove unused import:
sleap.gui.learning.runners
22-22:
sleap.nn.inference.InferenceLayerimported but unused (F401)Remove unused import
23-23:
sleap.nn.inference.InferenceModelimported but unused (F401)Remove unused import
56-56:
sleap.nn.tracking.MatchedFrameInstanceimported but unused (F401)Remove unused import:
sleap.nn.tracking.MatchedFrameInstance
61-61:
sleap.instance.Trackimported but unused (F401)Remove unused import:
sleap.instance.Track
608-608: Avoid equality comparisons to
False; useif not predictor.is_grayscale:for false checks (E712)Replace with
not predictor.is_grayscale
656-656: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
705-705: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
738-738: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
797-797: Pointless comparison. Did you mean to assign a value? Otherwise, prepend
assertor remove it. (B015)
798-798: Pointless comparison. Did you mean to assign a value? Otherwise, prepend
assertor remove it. (B015)
893-893: Local variable
predictoris assigned to but never used (F841)Remove assignment to unused variable
predictor
1252-1252:
pytest.raises(Exception)should be considered evil (B017)
1382-1382: Local variable
labelsis assigned to but never used (F841)Remove assignment to unused variable
labels
1664-1664: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1708-1708: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1713-1713: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1743-1743: Avoid equality comparisons to
False; useif not predictor.is_grayscale:for false checks (E712)Replace with
not predictor.is_grayscalesleap/nn/inference.py
131-131:
typing.Textis deprecated, usestr(UP019)Replace with
str
149-149: Undefined name
Task(F821)
584-584: Local variable
outputsis assigned to but never used (F841)Remove assignment to unused variable
outputs
605-605:
typing.Textis deprecated, usestr(UP019)Replace with
str
613-613:
typing.Textis deprecated, usestr(UP019)Replace with
str
627-627:
typing.Textis deprecated, usestr(UP019)Replace with
str
641-641:
typing.Textis deprecated, usestr(UP019)Replace with
str
695-696: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
1183-1183: No explicit
stacklevelkeyword argument found (B028)
1325-1328: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
1468-1468:
typing.Textis deprecated, usestr(UP019)Replace with
str
1758-1763: Use ternary operator
imgs = inputs["image"] if isinstance(inputs, dict) else inputsinstead ofif-else-block (SIM108)
2394-2394:
typing.Textis deprecated, usestr(UP019)Replace with
str
2395-2395:
typing.Textis deprecated, usestr(UP019)Replace with
str
2822-2825: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
3110-3110:
typing.Textis deprecated, usestr(UP019)Replace with
str
3424-3427: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
3436-3439: Use ternary operator
offsets = preds[self.offsets_ind] if self.offsets_ind is not None else Noneinstead ofif-else-block (SIM108)Replace
if-else-block withoffsets = preds[self.offsets_ind] if self.offsets_ind is not None else None
3654-3654:
typing.Textis deprecated, usestr(UP019)Replace with
str
3756-3756: Loop control variable
imagenot used within loop body (B007)Rename unused
imageto_image
4262-4262:
typing.Textis deprecated, usestr(UP019)Replace with
str
4263-4263:
typing.Textis deprecated, usestr(UP019)Replace with
str
4452-4452: Loop control variable
imagenot used within loop body (B007)Rename unused
imageto_image
4615-4619: Use ternary operator
img = ex["image"] if type(ex) == dict else exinstead ofif-else-block (SIM108)Replace
if-else-block withimg = ex["image"] if type(ex) == dict else ex
4615-4615: Do not compare types, use
isinstance()(E721)
4709-4709:
typing.Textis deprecated, usestr(UP019)Replace with
str
4873-4876: Use ternary operator
model_paths = [model_path] if isinstance(model_path, str) else model_pathinstead ofif-else-block (SIM108)Replace
if-else-block withmodel_paths = [model_path] if isinstance(model_path, str) else model_path
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-block (SIM108)Replace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)
5350-5350: Local variable
eis assigned to but never used (F841)Remove assignment to unused variable
e
Additional comments not posted (1)
tests/nn/test_inference.py (1)
67-77: The new fixtures introduced here are crucial for testing the multi-video inference capabilities. They correctly set up paths to test data for different input formats. Ensure that the paths are valid and the data is correctly formatted for these tests.
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
sleap/nn/inference.py (2)
5368-5368: Consider removing or guarding debug print statements in production code.It's generally good practice to avoid leaving debug prints in production code or guard them with a verbosity check.
5449-5449: Ensure that the debug prints are necessary or are appropriately gated by a debug or verbose flag.Consider wrapping debug statements with a verbosity check to avoid cluttering the output in production environments.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (5 hunks)
- tests/nn/test_inference.py (3 hunks)
Additional context used
Ruff
tests/nn/test_inference.py
10-10:
tensorflow_hubimported but unused (F401)Remove unused import:
tensorflow_hub
14-14:
sleap.gui.learning.runnersimported but unused (F401)Remove unused import:
sleap.gui.learning.runners
22-22:
sleap.nn.inference.InferenceLayerimported but unused (F401)Remove unused import
23-23:
sleap.nn.inference.InferenceModelimported but unused (F401)Remove unused import
56-56:
sleap.nn.tracking.MatchedFrameInstanceimported but unused (F401)Remove unused import:
sleap.nn.tracking.MatchedFrameInstance
61-61:
sleap.instance.Trackimported but unused (F401)Remove unused import:
sleap.instance.Track
612-612: Avoid equality comparisons to
False; useif not predictor.is_grayscale:for false checks (E712)Replace with
not predictor.is_grayscale
660-660: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
709-709: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
742-742: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
801-801: Pointless comparison. Did you mean to assign a value? Otherwise, prepend
assertor remove it. (B015)
802-802: Pointless comparison. Did you mean to assign a value? Otherwise, prepend
assertor remove it. (B015)
897-897: Local variable
predictoris assigned to but never used (F841)Remove assignment to unused variable
predictor
1256-1256:
pytest.raises(Exception)should be considered evil (B017)
1386-1386: Local variable
labelsis assigned to but never used (F841)Remove assignment to unused variable
labels
1708-1708: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1752-1752: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1757-1757: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1787-1787: Avoid equality comparisons to
False; useif not predictor.is_grayscale:for false checks (E712)Replace with
not predictor.is_grayscalesleap/nn/inference.py
131-131:
typing.Textis deprecated, usestr(UP019)Replace with
str
149-149: Undefined name
Task(F821)
584-584: Local variable
outputsis assigned to but never used (F841)Remove assignment to unused variable
outputs
605-605:
typing.Textis deprecated, usestr(UP019)Replace with
str
613-613:
typing.Textis deprecated, usestr(UP019)Replace with
str
627-627:
typing.Textis deprecated, usestr(UP019)Replace with
str
641-641:
typing.Textis deprecated, usestr(UP019)Replace with
str
695-696: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
1183-1183: No explicit
stacklevelkeyword argument found (B028)
1325-1328: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
1468-1468:
typing.Textis deprecated, usestr(UP019)Replace with
str
1758-1763: Use ternary operator
imgs = inputs["image"] if isinstance(inputs, dict) else inputsinstead ofif-else-block (SIM108)
2394-2394:
typing.Textis deprecated, usestr(UP019)Replace with
str
2395-2395:
typing.Textis deprecated, usestr(UP019)Replace with
str
2822-2825: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
3110-3110:
typing.Textis deprecated, usestr(UP019)Replace with
str
3424-3427: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
3436-3439: Use ternary operator
offsets = preds[self.offsets_ind] if self.offsets_ind is not None else Noneinstead ofif-else-block (SIM108)Replace
if-else-block withoffsets = preds[self.offsets_ind] if self.offsets_ind is not None else None
3654-3654:
typing.Textis deprecated, usestr(UP019)Replace with
str
3756-3756: Loop control variable
imagenot used within loop body (B007)Rename unused
imageto_image
4262-4262:
typing.Textis deprecated, usestr(UP019)Replace with
str
4263-4263:
typing.Textis deprecated, usestr(UP019)Replace with
str
4452-4452: Loop control variable
imagenot used within loop body (B007)Rename unused
imageto_image
4615-4619: Use ternary operator
img = ex["image"] if type(ex) == dict else exinstead ofif-else-block (SIM108)Replace
if-else-block withimg = ex["image"] if type(ex) == dict else ex
4615-4615: Do not compare types, use
isinstance()(E721)
4709-4709:
typing.Textis deprecated, usestr(UP019)Replace with
str
4873-4876: Use ternary operator
model_paths = [model_path] if isinstance(model_path, str) else model_pathinstead ofif-else-block (SIM108)Replace
if-else-block withmodel_paths = [model_path] if isinstance(model_path, str) else model_path
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-block (SIM108)Replace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)
5350-5350: Local variable
eis assigned to but never used (F841)Remove assignment to unused variable
e
Additional comments not posted (4)
tests/nn/test_inference.py (4)
66-69: LGTM! This fixture correctly provides a path for multiple.slpinputs as expected.
74-77: LGTM! This fixture properly returns a directory path for testing with both.slpand.mp4inputs.
78-81: LGTM! The fixture correctly provides a path for directories with multiple.mp4files, facilitating testing of the new multi-input feature.
1524-1637: Excellent work on testing the new multi-input functionality. The test is well-structured and covers the essential aspects of the feature by checking the existence of expected output files.
sleap/nn/inference.py
Outdated
|
|
||
| if args.open_in_gui: | ||
| subprocess.call(["sleap-label", output_path]) | ||
|
|
There was a problem hiding this comment.
Refine the error message to provide more context and guidance.
- "Use \"sleap-track --tracking.tracker ...' to specify tracker to use."
+ "Specify the tracker using 'sleap-track --tracking.tracker'. Refer to the documentation for more details."Committable suggestion was skipped due to low confidence.
sleap/nn/inference.py
Outdated
|
|
||
| # Figure out which input path to use. | ||
| labels_path = getattr(args, "labels", None) | ||
| if labels_path is not None: | ||
| data_path = labels_path | ||
| data_path = Path(labels_path) | ||
| else: | ||
| data_path = Path(args.data_path) | ||
|
|
There was a problem hiding this comment.
Consider using a ternary operator for cleaner and more concise code.
- if labels_path is not None:
- data_path = Path(labels_path)
- else:
- data_path = Path(args.data_path)
+ data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Figure out which input path to use. | |
| labels_path = getattr(args, "labels", None) | |
| if labels_path is not None: | |
| data_path = labels_path | |
| data_path = Path(labels_path) | |
| else: | |
| data_path = Path(args.data_path) | |
| # Figure out which input path to use. | |
| labels_path = getattr(args, "labels", None) | |
| data_path = Path(labels_path) if labels_path is not None else Path(args.data_path) | |
Tools
Ruff
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-block (SIM108)Replace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)
sleap/nn/inference.py
Outdated
| if output_path is not None and (Path.is_file(output_path_obj) and len(data_path_list) > 1): | ||
| raise ValueError( | ||
| "output_path argument must be a directory if multiple video inputs are given" | ||
| ) |
There was a problem hiding this comment.
Validate the output path more robustly to handle cases where the output path is not a directory but should be.
- if output_path is not None and (Path.is_file(output_path_obj) and len(data_path_list) > 1):
+ if output_path is not None and not Path(output_path_obj).is_dir() and len(data_path_list) > 1:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if output_path is not None and (Path.is_file(output_path_obj) and len(data_path_list) > 1): | |
| raise ValueError( | |
| "output_path argument must be a directory if multiple video inputs are given" | |
| ) | |
| if output_path is not None and not Path(output_path_obj).is_dir() and len(data_path_list) > 1: | |
| raise ValueError( | |
| "output_path argument must be a directory if multiple video inputs are given" | |
| ) |
sleap/nn/inference.py
Outdated
| # Check for multiple video inputs | ||
| # Compile file(s) into a list for later itteration | ||
| if data_path.is_dir: | ||
| data_path_list = [] | ||
| for file_path in data_path.iterdir(): | ||
| if file_path.is_file(): | ||
| data_path_list.append(file_path) | ||
| elif data_path.is_file: | ||
| data_path_list = [args.data_path] |
There was a problem hiding this comment.
Ensure is_dir is called as a method to check if the path is a directory.
- if data_path.is_dir:
+ if data_path.is_dir():Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check for multiple video inputs | |
| # Compile file(s) into a list for later itteration | |
| if data_path.is_dir: | |
| data_path_list = [] | |
| for file_path in data_path.iterdir(): | |
| if file_path.is_file(): | |
| data_path_list.append(file_path) | |
| elif data_path.is_file: | |
| data_path_list = [args.data_path] | |
| # Check for multiple video inputs | |
| # Compile file(s) into a list for later itteration | |
| if data_path.is_dir(): | |
| data_path_list = [] | |
| for file_path in data_path.iterdir(): | |
| if file_path.is_file(): | |
| data_path_list.append(file_path) | |
| elif data_path.is_file: | |
| data_path_list = [args.data_path] |
sleap/nn/inference.py
Outdated
| # Provider list to accomodate multiple video inputs | ||
| provider_list = [] | ||
| tmp_data_path_list = [] | ||
| for data_path_file in data_path_list: | ||
| data_path_file = Path(data_path_file) | ||
| # Create a provider for each file | ||
| if data_path_file.as_posix().endswith(".slp"): | ||
| print(f"Sleap file: {data_path_file}") | ||
| labels = sleap.load_file(data_path_file.as_posix()) | ||
|
|
||
| if args.only_labeled_frames: | ||
| provider_list.append(LabelsReader.from_user_labeled_frames(labels)) | ||
| elif args.only_suggested_frames: | ||
| provider_list.append(LabelsReader.from_unlabeled_suggestions(labels)) | ||
| elif getattr(args, "video.index") != "": | ||
| provider_list.append(VideoReader( | ||
| video=labels.videos[int(getattr(args, "video.index"))], | ||
| example_indices=frame_list(args.frames), | ||
| )) | ||
| else: | ||
| provider_list.append(LabelsReader(labels)) | ||
|
|
||
| tmp_data_path_list.append(data_path_file) | ||
|
|
||
| else: | ||
| print(f"Video: {data_path}") | ||
| # TODO: Clean this up. | ||
| video_kwargs = dict( | ||
| dataset=vars(args).get("video.dataset"), | ||
| input_format=vars(args).get("video.input_format"), | ||
| ) | ||
| provider = VideoReader.from_filepath( | ||
| filename=data_path, example_indices=frame_list(args.frames), **video_kwargs | ||
| ) | ||
| else: | ||
| try: | ||
| video_kwargs = dict( | ||
| dataset=vars(args).get("video.dataset"), | ||
| input_format=vars(args).get("video.input_format"), | ||
| ) | ||
| provider_list.append(VideoReader.from_filepath( | ||
| filename=data_path_file.as_posix(), example_indices=frame_list(args.frames), **video_kwargs | ||
| )) | ||
| print(f"Video: {data_path_file}") | ||
| tmp_data_path_list.append(data_path_file) | ||
| # TODO: Clean this up. | ||
| except Exception as e: | ||
| print(f"Error reading file: {data_path_file}") | ||
|
|
||
| data_path_list = tmp_data_path_list | ||
|
|
There was a problem hiding this comment.
Refactor exception handling to catch specific exceptions and remove the assignment to the unused variable e.
- except Exception as e:
+ except (FileNotFoundError, PermissionError):
- print(f"Error reading file: {data_path_file}")
+ print(f"Error reading file: {data_path_file}. Please check the file's existence and permissions.")
- except Exception as e:
+ except Exception:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Provider list to accomodate multiple video inputs | |
| provider_list = [] | |
| tmp_data_path_list = [] | |
| for data_path_file in data_path_list: | |
| data_path_file = Path(data_path_file) | |
| # Create a provider for each file | |
| if data_path_file.as_posix().endswith(".slp"): | |
| print(f"Sleap file: {data_path_file}") | |
| labels = sleap.load_file(data_path_file.as_posix()) | |
| if args.only_labeled_frames: | |
| provider_list.append(LabelsReader.from_user_labeled_frames(labels)) | |
| elif args.only_suggested_frames: | |
| provider_list.append(LabelsReader.from_unlabeled_suggestions(labels)) | |
| elif getattr(args, "video.index") != "": | |
| provider_list.append(VideoReader( | |
| video=labels.videos[int(getattr(args, "video.index"))], | |
| example_indices=frame_list(args.frames), | |
| )) | |
| else: | |
| provider_list.append(LabelsReader(labels)) | |
| tmp_data_path_list.append(data_path_file) | |
| else: | |
| print(f"Video: {data_path}") | |
| # TODO: Clean this up. | |
| video_kwargs = dict( | |
| dataset=vars(args).get("video.dataset"), | |
| input_format=vars(args).get("video.input_format"), | |
| ) | |
| provider = VideoReader.from_filepath( | |
| filename=data_path, example_indices=frame_list(args.frames), **video_kwargs | |
| ) | |
| else: | |
| try: | |
| video_kwargs = dict( | |
| dataset=vars(args).get("video.dataset"), | |
| input_format=vars(args).get("video.input_format"), | |
| ) | |
| provider_list.append(VideoReader.from_filepath( | |
| filename=data_path_file.as_posix(), example_indices=frame_list(args.frames), **video_kwargs | |
| )) | |
| print(f"Video: {data_path_file}") | |
| tmp_data_path_list.append(data_path_file) | |
| # TODO: Clean this up. | |
| except Exception as e: | |
| print(f"Error reading file: {data_path_file}") | |
| data_path_list = tmp_data_path_list | |
| # Provider list to accomodate multiple video inputs | |
| provider_list = [] | |
| tmp_data_path_list = [] | |
| for data_path_file in data_path_list: | |
| data_path_file = Path(data_path_file) | |
| # Create a provider for each file | |
| if data_path_file.as_posix().endswith(".slp"): | |
| print(f"Sleap file: {data_path_file}") | |
| labels = sleap.load_file(data_path_file.as_posix()) | |
| if args.only_labeled_frames: | |
| provider_list.append(LabelsReader.from_user_labeled_frames(labels)) | |
| elif args.only_suggested_frames: | |
| provider_list.append(LabelsReader.from_unlabeled_suggestions(labels)) | |
| elif getattr(args, "video.index") != "": | |
| provider_list.append(VideoReader( | |
| video=labels.videos[int(getattr(args, "video.index"))], | |
| example_indices=frame_list(args.frames), | |
| )) | |
| else: | |
| provider_list.append(LabelsReader(labels)) | |
| tmp_data_path_list.append(data_path_file) | |
| else: | |
| try: | |
| video_kwargs = dict( | |
| dataset=vars(args).get("video.dataset"), | |
| input_format=vars(args).get("video.input_format"), | |
| ) | |
| provider_list.append(VideoReader.from_filepath( | |
| filename=data_path_file.as_posix(), example_indices=frame_list(args.frames), **video_kwargs | |
| )) | |
| print(f"Video: {data_path_file}") | |
| tmp_data_path_list.append(data_path_file) | |
| # TODO: Clean this up. | |
| except (FileNotFoundError, PermissionError): | |
| print(f"Error reading file: {data_path_file}. Please check the file's existence and permissions.") | |
| except Exception: | |
| print(f"Error reading file: {data_path_file}") | |
| data_path_list = tmp_data_path_list |
Tools
Ruff
5350-5350: Local variable
eis assigned to but never used (F841)Remove assignment to unused variable
e
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (10)
tests/nn/test_inference.py (10)
Line range hint
10-10: Remove unused import: tensorflow_hubThe import
tensorflow_hubis not used anywhere in the code. To maintain clean and efficient code, it's best to remove unused imports.- import tensorflow_hub as hub
Line range hint
14-14: Remove unused import:sleap.gui.learning.runnersThis import is not utilized in the file and should be removed to keep the codebase clean and efficient.
- from sleap.gui.learning import runners
Line range hint
22-23: Remove unused imports related to inferenceThe imports
InferenceLayerandInferenceModelfromsleap.nn.inferenceare not used in this file. Removing unused imports can help reduce confusion and improve code cleanliness.- from sleap.nn.inference import InferenceLayer - from sleap.nn.inference import InferenceModel
Line range hint
56-56: Remove unused import:sleap.nn.tracking.MatchedFrameInstanceThis import is not utilized in the file and should be removed to keep the codebase clean and efficient.
- from sleap.nn.tracking import MatchedFrameInstance
Line range hint
61-61: Remove unused import:sleap.instance.TrackThis import is not utilized in the file and should be removed to keep the codebase clean and efficient.
- from sleap.instance import Track
Line range hint
616-616: Refactor boolean checks for clarityBoolean comparisons to
TrueorFalseare not necessary and can be simplified for better readability and performance.- if predictor.is_grayscale == False: + if not predictor.is_grayscale: - if predictor.is_grayscale == True: + if predictor.is_grayscale:Also applies to: 664-664, 713-713, 746-746, 1821-1821
Line range hint
901-901: Remove unused variable:predictorThe variable
predictoris assigned but never used, which can lead to unnecessary memory usage and potential confusion.- predictor = load_model(model_path)
Line range hint
1260-1260: Avoid general exception types in testsUsing
pytest.raises(Exception)is too broad and can potentially catch unintended exceptions. It's recommended to specify the exact exception type expected.- with pytest.raises(Exception): + with pytest.raises(SpecificExceptionType):
Line range hint
1390-1390: Remove unused variable:labelsThe variable
labelsis assigned but never used, which can lead to unnecessary memory usage and potential confusion.- labels = Labels(min_tracks_2node_labels[[0]])
Line range hint
1742-1742: Simplify dictionary key checksUsing
key in dict.keys()is redundant and can be simplified tokey in dictfor clarity and efficiency.- for key in tracker.track_matching_queue_dict.keys(): + for key in tracker.track_matching_queue_dict:Also applies to: 1786-1786, 1791-1791
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (5 hunks)
- tests/nn/test_inference.py (3 hunks)
Additional context used
Ruff
tests/nn/test_inference.py
10-10:
tensorflow_hubimported but unused (F401)Remove unused import:
tensorflow_hub
14-14:
sleap.gui.learning.runnersimported but unused (F401)Remove unused import:
sleap.gui.learning.runners
22-22:
sleap.nn.inference.InferenceLayerimported but unused (F401)Remove unused import
23-23:
sleap.nn.inference.InferenceModelimported but unused (F401)Remove unused import
56-56:
sleap.nn.tracking.MatchedFrameInstanceimported but unused (F401)Remove unused import:
sleap.nn.tracking.MatchedFrameInstance
61-61:
sleap.instance.Trackimported but unused (F401)Remove unused import:
sleap.instance.Track
616-616: Avoid equality comparisons to
False; useif not predictor.is_grayscale:for false checks (E712)Replace with
not predictor.is_grayscale
664-664: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
713-713: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
746-746: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checks (E712)Replace with
predictor.is_grayscale
805-805: Pointless comparison. Did you mean to assign a value? Otherwise, prepend
assertor remove it. (B015)
806-806: Pointless comparison. Did you mean to assign a value? Otherwise, prepend
assertor remove it. (B015)
901-901: Local variable
predictoris assigned to but never used (F841)Remove assignment to unused variable
predictor
1260-1260:
pytest.raises(Exception)should be considered evil (B017)
1390-1390: Local variable
labelsis assigned to but never used (F841)Remove assignment to unused variable
labels
1742-1742: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1786-1786: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1791-1791: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
1821-1821: Avoid equality comparisons to
False; useif not predictor.is_grayscale:for false checks (E712)Replace with
not predictor.is_grayscalesleap/nn/inference.py
131-131:
typing.Textis deprecated, usestr(UP019)Replace with
str
149-149: Undefined name
Task(F821)
584-584: Local variable
outputsis assigned to but never used (F841)Remove assignment to unused variable
outputs
605-605:
typing.Textis deprecated, usestr(UP019)Replace with
str
613-613:
typing.Textis deprecated, usestr(UP019)Replace with
str
627-627:
typing.Textis deprecated, usestr(UP019)Replace with
str
641-641:
typing.Textis deprecated, usestr(UP019)Replace with
str
695-696: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
1183-1183: No explicit
stacklevelkeyword argument found (B028)
1325-1328: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
1468-1468:
typing.Textis deprecated, usestr(UP019)Replace with
str
1758-1763: Use ternary operator
imgs = inputs["image"] if isinstance(inputs, dict) else inputsinstead ofif-else-block (SIM108)
2394-2394:
typing.Textis deprecated, usestr(UP019)Replace with
str
2395-2395:
typing.Textis deprecated, usestr(UP019)Replace with
str
2822-2825: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
3110-3110:
typing.Textis deprecated, usestr(UP019)Replace with
str
3424-3427: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
3436-3439: Use ternary operator
offsets = preds[self.offsets_ind] if self.offsets_ind is not None else Noneinstead ofif-else-block (SIM108)Replace
if-else-block withoffsets = preds[self.offsets_ind] if self.offsets_ind is not None else None
3654-3654:
typing.Textis deprecated, usestr(UP019)Replace with
str
3756-3756: Loop control variable
imagenot used within loop body (B007)Rename unused
imageto_image
4262-4262:
typing.Textis deprecated, usestr(UP019)Replace with
str
4263-4263:
typing.Textis deprecated, usestr(UP019)Replace with
str
4452-4452: Loop control variable
imagenot used within loop body (B007)Rename unused
imageto_image
4615-4619: Use ternary operator
img = ex["image"] if type(ex) == dict else exinstead ofif-else-block (SIM108)Replace
if-else-block withimg = ex["image"] if type(ex) == dict else ex
4615-4615: Do not compare types, use
isinstance()(E721)
4709-4709:
typing.Textis deprecated, usestr(UP019)Replace with
str
4873-4876: Use ternary operator
model_paths = [model_path] if isinstance(model_path, str) else model_pathinstead ofif-else-block (SIM108)Replace
if-else-block withmodel_paths = [model_path] if isinstance(model_path, str) else model_path
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-block (SIM108)Replace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)
5356-5356: Local variable
eis assigned to but never used (F841)Remove assignment to unused variable
e
Additional comments not posted (3)
sleap/nn/inference.py (3)
5373-5373: Consider removing debug statements.The
print(args)statement appears to be for debugging purposes. If it's no longer needed, consider removing it to clean up the code.- print(args)
5454-5454: Review necessity of verbose argument logging.Printing all arguments might expose sensitive information in logs and could clutter the output. Ensure that this is necessary, or consider logging at a debug level or to a file that is not generally accessible.
5503-5510: Error handling for output path conflicts is robust.The check to ensure the
output_pathis a directory when multiple inputs are provided is crucial for preventing file overwriting and data loss. This is a good practice in handling user input for batch processing scenarios.
sleap/nn/inference.py
Outdated
| # Compile file(s) into a list for later itteration | ||
| if data_path.is_dir(): | ||
| data_path_list = [] | ||
| for file_path in data_path.iterdir(): | ||
| if file_path.is_file(): | ||
| data_path_list.append(file_path) | ||
| elif data_path.is_file: | ||
| data_path_list = [args.data_path] |
There was a problem hiding this comment.
Implementation of multiple video input handling appears robust.
The code effectively handles multiple video inputs by checking if the path is a directory and iterating over all files. This implementation aligns with the PR's objective to allow sleap-track to process multiple videos. The use of lists to manage providers and paths is appropriate, and the error handling for unsupported paths is a good practice.
However, the use of data_path.is_file without calling it (i.e., data_path.is_file()) appears to be a typo that could lead to unexpected behavior.
- elif data_path.is_file:
+ elif data_path.is_file():Also applies to: 5314-5361
sleap/nn/inference.py
Outdated
| # Run inference on all files inputed | ||
| for data_path, provider in zip(data_path_list, provider_list): | ||
| # Setup models. | ||
| predictor = _make_predictor_from_cli(args) | ||
| print(f"predictor.tracker: {tracker}") | ||
| predictor.tracker = tracker | ||
|
|
||
| if output_path is None: | ||
| output_path = data_path + ".predictions.slp" | ||
| # Run inference! | ||
| labels_pr = predictor.predict(provider) | ||
|
|
||
| labels_pr.provenance["model_paths"] = predictor.model_paths | ||
| labels_pr.provenance["predictor"] = type(predictor).__name__ | ||
| if output_path is None: | ||
|
|
||
| output_path = data_path.parent / (data_path.stem + ".predictions.slp") | ||
| output_path_obj = Path(output_path) | ||
|
|
||
| else: | ||
| output_path = output_path + "/" + (data_path.stem + ".predictions.slp") | ||
|
|
||
| labels_pr.provenance["model_paths"] = predictor.model_paths | ||
| labels_pr.provenance["predictor"] = type(predictor).__name__ | ||
|
|
||
| if args.no_empty_frames: | ||
| # Clear empty frames if specified. | ||
| labels_pr.remove_empty_frames() | ||
|
|
||
| finish_timestamp = str(datetime.now()) | ||
| total_elapsed = time() - t0 | ||
| print("Finished inference at:", finish_timestamp) | ||
| print(f"Total runtime: {total_elapsed} secs") | ||
| print(f"Predicted frames: {len(labels_pr)}/{len(provider)}") | ||
|
|
||
| # Add provenance metadata to predictions. | ||
| labels_pr.provenance["sleap_version"] = sleap.__version__ | ||
| labels_pr.provenance["platform"] = platform.platform() | ||
| labels_pr.provenance["command"] = " ".join(sys.argv) | ||
| labels_pr.provenance["data_path"] = data_path.as_posix() | ||
| labels_pr.provenance["output_path"] = output_path_obj.as_posix() | ||
| labels_pr.provenance["total_elapsed"] = total_elapsed | ||
| labels_pr.provenance["start_timestamp"] = start_timestamp | ||
| labels_pr.provenance["finish_timestamp"] = finish_timestamp | ||
|
|
||
| print("Provenance:") | ||
| print(labels_pr.provenance) | ||
| print() | ||
|
|
||
| labels_pr.provenance["args"] = vars(args) | ||
|
|
||
| # Save results. | ||
| labels_pr.save(output_path) | ||
| print("Saved output:", output_path) | ||
|
|
||
| if args.open_in_gui: | ||
| subprocess.call(["sleap-label", output_path]) | ||
|
|
||
| # Reset output_path for next iteration | ||
| output_path = args.output | ||
|
|
There was a problem hiding this comment.
Ensure consistency in output path handling.
The loop handles inference for multiple files, but the handling of output_path could lead to inconsistencies or errors if not managed correctly, especially when it's reset at the end of the loop. Consider encapsulating output path generation and handling in a separate function to reduce complexity and improve maintainability.
def generate_output_path(data_path, output_dir, suffix=".predictions.slp"):
return output_dir / (data_path.stem + suffix)
# Usage within the loop
output_path = generate_output_path(data_path, output_path_obj)
sleap/nn/inference.py
Outdated
| except Exception as e: | ||
| print(f"Error reading file: {data_path_file}") |
There was a problem hiding this comment.
Remove unused variable and improve error handling.
The variable e in the exception block is unused, which can be cleaned up. Additionally, consider logging the error or providing more detailed feedback to the user.
- except Exception as e:
- print(f"Error reading file: {data_path_file}")
+ except Exception:
+ print(f"Error reading file: {data_path_file}. Please check the file format and accessibility.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| print(f"Error reading file: {data_path_file}") | |
| except Exception: | |
| print(f"Error reading file: {data_path_file}. Please check the file format and accessibility.") |
Tools
Ruff
5356-5356: Local variable
eis assigned to but never used (F841)Remove assignment to unused variable
e
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/nn/inference.py (5 hunks)
Additional context used
Ruff
sleap/nn/inference.py
131-131:
typing.Textis deprecated, usestr(UP019)Replace with
str
149-149: Undefined name
Task(F821)
584-584: Local variable
outputsis assigned to but never used (F841)Remove assignment to unused variable
outputs
605-605:
typing.Textis deprecated, usestr(UP019)Replace with
str
613-613:
typing.Textis deprecated, usestr(UP019)Replace with
str
627-627:
typing.Textis deprecated, usestr(UP019)Replace with
str
641-641:
typing.Textis deprecated, usestr(UP019)Replace with
str
695-696: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
1183-1183: No explicit
stacklevelkeyword argument found (B028)
1325-1328: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
1468-1468:
typing.Textis deprecated, usestr(UP019)Replace with
str
1758-1763: Use ternary operator
imgs = inputs["image"] if isinstance(inputs, dict) else inputsinstead ofif-else-block (SIM108)
2394-2394:
typing.Textis deprecated, usestr(UP019)Replace with
str
2395-2395:
typing.Textis deprecated, usestr(UP019)Replace with
str
2822-2825: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
3110-3110:
typing.Textis deprecated, usestr(UP019)Replace with
str
3424-3427: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-block (SIM108)Replace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data
3436-3439: Use ternary operator
offsets = preds[self.offsets_ind] if self.offsets_ind is not None else Noneinstead ofif-else-block (SIM108)Replace
if-else-block withoffsets = preds[self.offsets_ind] if self.offsets_ind is not None else None
3654-3654:
typing.Textis deprecated, usestr(UP019)Replace with
str
3756-3756: Loop control variable
imagenot used within loop body (B007)Rename unused
imageto_image
4262-4262:
typing.Textis deprecated, usestr(UP019)Replace with
str
4263-4263:
typing.Textis deprecated, usestr(UP019)Replace with
str
4452-4452: Loop control variable
imagenot used within loop body (B007)Rename unused
imageto_image
4615-4619: Use ternary operator
img = ex["image"] if type(ex) == dict else exinstead ofif-else-block (SIM108)Replace
if-else-block withimg = ex["image"] if type(ex) == dict else ex
4615-4615: Do not compare types, use
isinstance()(E721)
4709-4709:
typing.Textis deprecated, usestr(UP019)Replace with
str
4873-4876: Use ternary operator
model_paths = [model_path] if isinstance(model_path, str) else model_pathinstead ofif-else-block (SIM108)Replace
if-else-block withmodel_paths = [model_path] if isinstance(model_path, str) else model_path
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-block (SIM108)Replace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)
Additional comments not posted (3)
sleap/nn/inference.py (3)
5504-5509: Validate output path for multiple inputs.The validation for
output_pathwhen multiple video inputs are provided is crucial. Ensure that this logic is robust and tested to prevent runtime errors.
5575-5626: Refactor tracking logic for clarity.The tracking logic, especially with the handling of existing prediction files, could benefit from refactoring for better clarity and maintainability. Consider separating the concerns into more discrete functions or methods.
[REFACTOR_Suggestion]def handle_existing_predictions(data_path, provider, tracker, args): print("Loading predictions...") labels_pr = sleap.load_file(args.data_path.as_posix()) frames = sorted(labels_pr.labeled_frames, key=lambda lf: lf.frame_idx) print("Starting tracker...") frames = run_tracker(frames=frames, tracker=tracker) tracker.final_pass(frames) return Labels(labeled_frames=frames)
5524-5568: Review inference execution and result handling.The inference execution and handling of results appear to be implemented correctly. However, ensure that all edge cases are covered, especially with file paths and output handling.
Verification successful
Edge cases for file paths and output handling are covered.
The test case
test_sleap_track_output_multintests/nn/test_inference.pyverifies the correct construction of output file paths and ensures that predictions files are created as expected. It checks for the existence of the output files and handles different video formats.
tests/nn/test_inference.py:
test_sleap_track_output_multScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all edge cases for file paths and output handling are covered in tests. # Test: Search for test cases covering file paths and output handling. rg --type python "test_sleap_track_output_mult"Length of output: 78
Script:
#!/bin/bash # Description: Verify that all edge cases for file paths and output handling are covered in tests. # Correcting the file type argument to 'py' instead of 'python'. rg --type py "test_sleap_track_output_mult"Length of output: 104
Script:
#!/bin/bash # Description: Extract and review the identified test case to ensure it covers edge cases for file paths and output handling. # Extract the content of the identified test case. rg --type py -A 20 "def test_sleap_track_output_mult" tests/nn/test_inference.pyLength of output: 821
Script:
#!/bin/bash # Description: Extract and review the identified test case to ensure it covers edge cases for file paths and output handling. # Extract more lines of the identified test case for comprehensive review. rg --type py -A 50 "def test_sleap_track_output_mult" tests/nn/test_inference.pyLength of output: 1749
sleap/nn/inference.py
Outdated
| if data_path_file.as_posix().endswith(".slp"): | ||
| print(f"Sleap file: {data_path_file}") | ||
| labels = sleap.load_file(data_path_file.as_posix()) | ||
|
|
||
| if args.only_labeled_frames: | ||
| provider_list.append(LabelsReader.from_user_labeled_frames(labels)) | ||
| elif args.only_suggested_frames: | ||
| provider_list.append(LabelsReader.from_unlabeled_suggestions(labels)) | ||
| elif getattr(args, "video.index") != "": | ||
| provider_list.append( | ||
| VideoReader( | ||
| video=labels.videos[int(getattr(args, "video.index"))], | ||
| example_indices=frame_list(args.frames), | ||
| ) | ||
| ) | ||
| else: | ||
| provider_list.append(LabelsReader(labels)) | ||
|
|
There was a problem hiding this comment.
Consider refactoring provider creation for clarity and modularity.
The code for creating providers based on file types is spread across multiple conditional branches. Consider refactoring this into a separate function or method to improve clarity and maintainability.
def create_provider(data_path_file, args):
if data_path_file.as_posix().endswith(".slp"):
labels = sleap.load_file(data_path_file.as_posix())
# Additional conditions based on args
else:
video_kwargs = dict(
dataset=vars(args).get("video.dataset"),
input_format=vars(args).get("video.input_format"),
)
return VideoReader.from_filepath(filename=data_path_file.as_posix(), example_indices=frame_list(args.frames), **video_kwargs)
return None # or appropriate returnAlso applies to: 5347-5352
sleap/nn/inference.py
Outdated
| except Exception: | ||
| print(f"Error reading file: {data_path_file}") |
There was a problem hiding this comment.
Improve error handling in file reading.
Currently, the error handling in the file reading process just prints an error message. It might be beneficial to either raise an exception or handle the error in a way that doesn't stop the execution, depending on the intended behavior of the application.
- except Exception:
- print(f"Error reading file: {data_path_file}")
+ except Exception as e:
+ logging.error(f"Failed to read file {data_path_file}: {e}")
+ continue # or other handling logicCommittable suggestion was skipped due to low confidence.
sleap/nn/inference.py
Outdated
| if data_path.is_dir(): | ||
| data_path_list = [] | ||
| for file_path in data_path.iterdir(): | ||
| if file_path.is_file(): | ||
| data_path_list.append(file_path) | ||
| elif data_path.is_file(): | ||
| data_path_list = [args.data_path] |
There was a problem hiding this comment.
Ensure robust directory and file handling.
When handling directories and files, it's crucial to ensure that the code robustly checks for valid directories and handles potential errors gracefully. Consider adding more specific error handling for directory-related issues.
+ if not data_path.exists():
+ raise FileNotFoundError(f"The specified path does not exist: {data_path}")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if data_path.is_dir(): | |
| data_path_list = [] | |
| for file_path in data_path.iterdir(): | |
| if file_path.is_file(): | |
| data_path_list.append(file_path) | |
| elif data_path.is_file(): | |
| data_path_list = [args.data_path] | |
| if not data_path.exists(): | |
| raise FileNotFoundError(f"The specified path does not exist: {data_path}") | |
| if data_path.is_dir(): | |
| data_path_list = [] | |
| for file_path in data_path.iterdir(): | |
| if file_path.is_file(): | |
| data_path_list.append(file_path) | |
| elif data_path.is_file(): | |
| data_path_list = [args.data_path] |
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/nn/inference.py (5 hunks)
Additional context used
Ruff
sleap/nn/inference.py
131-131:
typing.Textis deprecated, usestrReplace with
str(UP019)
149-149: Undefined name
Task(F821)
584-584: Local variable
outputsis assigned to but never usedRemove assignment to unused variable
outputs(F841)
605-605:
typing.Textis deprecated, usestrReplace with
str(UP019)
613-613:
typing.Textis deprecated, usestrReplace with
str(UP019)
627-627:
typing.Textis deprecated, usestrReplace with
str(UP019)
641-641:
typing.Textis deprecated, usestrReplace with
str(UP019)
695-696: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
1183-1183: No explicit
stacklevelkeyword argument found(B028)
1325-1328: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-blockReplace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data(SIM108)
1468-1468:
typing.Textis deprecated, usestrReplace with
str(UP019)
1758-1763: Use ternary operator
imgs = inputs["image"] if isinstance(inputs, dict) else inputsinstead ofif-else-block(SIM108)
2394-2394:
typing.Textis deprecated, usestrReplace with
str(UP019)
2395-2395:
typing.Textis deprecated, usestrReplace with
str(UP019)
2822-2825: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-blockReplace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data(SIM108)
3110-3110:
typing.Textis deprecated, usestrReplace with
str(UP019)
3424-3427: Use ternary operator
imgs = data["image"] if isinstance(data, dict) else datainstead ofif-else-blockReplace
if-else-block withimgs = data["image"] if isinstance(data, dict) else data(SIM108)
3436-3439: Use ternary operator
offsets = preds[self.offsets_ind] if self.offsets_ind is not None else Noneinstead ofif-else-blockReplace
if-else-block withoffsets = preds[self.offsets_ind] if self.offsets_ind is not None else None(SIM108)
3654-3654:
typing.Textis deprecated, usestrReplace with
str(UP019)
3756-3756: Loop control variable
imagenot used within loop bodyRename unused
imageto_image(B007)
4262-4262:
typing.Textis deprecated, usestrReplace with
str(UP019)
4263-4263:
typing.Textis deprecated, usestrReplace with
str(UP019)
4452-4452: Loop control variable
imagenot used within loop bodyRename unused
imageto_image(B007)
4615-4619: Use ternary operator
img = ex["image"] if type(ex) == dict else exinstead ofif-else-blockReplace
if-else-block withimg = ex["image"] if type(ex) == dict else ex(SIM108)
4615-4615: Do not compare types, use
isinstance()(E721)
4709-4709:
typing.Textis deprecated, usestrReplace with
str(UP019)
4873-4876: Use ternary operator
model_paths = [model_path] if isinstance(model_path, str) else model_pathinstead ofif-else-blockReplace
if-else-block withmodel_paths = [model_path] if isinstance(model_path, str) else model_path(SIM108)
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-blockReplace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)(SIM108)
sleap/nn/inference.py
Outdated
| if data_path.is_dir(): | ||
| data_path_list = [] | ||
| for file_path in data_path.iterdir(): | ||
| if file_path.is_file(): | ||
| data_path_list.append(Path(file_path)) | ||
| elif data_path.is_file(): | ||
| data_path_list = [data_path] | ||
| else: | ||
| data_path = args.data_path | ||
|
|
||
| if data_path is None or data_path == "": | ||
| raise ValueError( | ||
| "You must specify a path to a video or a labels dataset. " | ||
| "Run 'sleap-track -h' to see full command documentation." | ||
| ) | ||
|
|
||
| if data_path.endswith(".slp"): | ||
| labels = sleap.load_file(data_path) | ||
|
|
||
| if args.only_labeled_frames: | ||
| provider = LabelsReader.from_user_labeled_frames(labels) | ||
| elif args.only_suggested_frames: | ||
| provider = LabelsReader.from_unlabeled_suggestions(labels) | ||
| elif getattr(args, "video.index") != "": | ||
| provider = VideoReader( | ||
| video=labels.videos[int(getattr(args, "video.index"))], | ||
| example_indices=frame_list(args.frames), | ||
| ) | ||
| # Provider list to accomodate multiple video inputs | ||
| provider_list = [] | ||
| tmp_data_path_list = [] | ||
| for data_path_file in data_path_list: | ||
| # Create a provider for each file | ||
| if data_path_file.as_posix().endswith(".slp"): | ||
| print(f"Sleap file: {data_path_file}") | ||
| labels = sleap.load_file(data_path_file.as_posix()) | ||
|
|
||
| if args.only_labeled_frames: | ||
| provider_list.append(LabelsReader.from_user_labeled_frames(labels)) | ||
| elif args.only_suggested_frames: | ||
| provider_list.append(LabelsReader.from_unlabeled_suggestions(labels)) | ||
| elif getattr(args, "video.index") != "": | ||
| provider_list.append( | ||
| VideoReader( | ||
| video=labels.videos[int(getattr(args, "video.index"))], | ||
| example_indices=frame_list(args.frames), | ||
| ) | ||
| ) | ||
| else: | ||
| provider_list.append(LabelsReader(labels)) | ||
|
|
||
| tmp_data_path_list.append(data_path_file) | ||
|
|
||
| else: | ||
| provider = LabelsReader(labels) | ||
| try: | ||
| video_kwargs = dict( | ||
| dataset=vars(args).get("video.dataset"), | ||
| input_format=vars(args).get("video.input_format"), | ||
| ) | ||
| provider_list.append( | ||
| VideoReader.from_filepath( | ||
| filename=data_path_file.as_posix(), | ||
| example_indices=frame_list(args.frames), | ||
| **video_kwargs, | ||
| ) | ||
| ) | ||
| print(f"Video: {data_path_file}") | ||
| tmp_data_path_list.append(data_path_file) | ||
| # TODO: Clean this up. | ||
| except Exception: | ||
| print(f"Error reading file: {data_path_file}") | ||
|
|
||
| else: | ||
| print(f"Video: {data_path}") | ||
| # TODO: Clean this up. | ||
| video_kwargs = dict( | ||
| dataset=vars(args).get("video.dataset"), | ||
| input_format=vars(args).get("video.input_format"), | ||
| ) | ||
| provider = VideoReader.from_filepath( | ||
| filename=data_path, example_indices=frame_list(args.frames), **video_kwargs | ||
| ) | ||
| data_path_list = tmp_data_path_list | ||
|
|
||
| return provider, data_path | ||
| return provider_list, data_path_list |
There was a problem hiding this comment.
Logic to handle multiple video inputs looks good.
The logic to compile files into a list for iteration and the creation of providers based on file types are well implemented. However, consider refactoring the provider creation into a separate function for better modularity and clarity.
def create_provider(data_path_file, args):
if data_path_file.as_posix().endswith(".slp"):
labels = sleap.load_file(data_path_file.as_posix())
# Additional conditions based on args
else:
video_kwargs = dict(
dataset=vars(args).get("video.dataset"),
input_format=vars(args).get("video.input_format"),
)
return VideoReader.from_filepath(filename=data_path_file.as_posix(), example_indices=frame_list(args.frames), **video_kwargs)
return None # or appropriate return| # Reset output_path for next iteration | ||
| output_path = args.output | ||
|
|
||
| # running tracking on existing prediction file |
There was a problem hiding this comment.
Improve error handling for missing tracker specification.
The error message is clear, but consider providing more guidance or a direct link to documentation on how to specify a tracker.
raise ValueError(
"To retrack on predictions, must specify tracker. "
"Use 'sleap-track --tracking.tracker ...' to specify tracker to use. "
"See [documentation link] for more details."
)
sleap/nn/inference.py
Outdated
| labels_pr.provenance["predictor"] = type(predictor).__name__ | ||
|
|
||
| if args.no_empty_frames: | ||
| # Clear empty frames if specified. | ||
| labels_pr.remove_empty_frames() | ||
|
|
||
| finish_timestamp = str(datetime.now()) | ||
| total_elapsed = time() - t0 | ||
| print("Finished inference at:", finish_timestamp) | ||
| print(f"Total runtime: {total_elapsed} secs") | ||
| print(f"Predicted frames: {len(labels_pr)}/{len(provider)}") | ||
|
|
||
| # Add provenance metadata to predictions. | ||
| labels_pr.provenance["sleap_version"] = sleap.__version__ | ||
| labels_pr.provenance["platform"] = platform.platform() | ||
| labels_pr.provenance["command"] = " ".join(sys.argv) | ||
| labels_pr.provenance["data_path"] = data_path.as_posix() | ||
| labels_pr.provenance["output_path"] = output_path_obj.as_posix() | ||
| labels_pr.provenance["total_elapsed"] = total_elapsed | ||
| labels_pr.provenance["start_timestamp"] = start_timestamp | ||
| labels_pr.provenance["finish_timestamp"] = finish_timestamp | ||
|
|
||
| print("Provenance:") | ||
| print(labels_pr.provenance) | ||
| print() | ||
|
|
||
| labels_pr.provenance["args"] = vars(args) | ||
|
|
||
| # Save results. | ||
| labels_pr.save(output_path) | ||
| print("Saved output:", output_path) | ||
|
|
||
| if args.open_in_gui: | ||
| subprocess.call(["sleap-label", output_path]) | ||
|
|
||
| # Reset output_path for next iteration | ||
| output_path = args.output | ||
|
|
||
| # running tracking on existing prediction file | ||
| elif getattr(args, "tracking.tracker") is not None: | ||
| # Load predictions | ||
| print("Loading predictions...") | ||
| labels_pr = sleap.load_file(args.data_path) | ||
| frames = sorted(labels_pr.labeled_frames, key=lambda lf: lf.frame_idx) | ||
| for data_path, provider in zip(data_path_list, provider_list): | ||
| # Load predictions | ||
| print("Loading predictions...") | ||
| labels_pr = sleap.load_file(data_path.as_posix()) | ||
| frames = sorted(labels_pr.labeled_frames, key=lambda lf: lf.frame_idx) | ||
|
|
||
| print("Starting tracker...") | ||
| frames = run_tracker(frames=frames, tracker=tracker) | ||
| tracker.final_pass(frames) | ||
|
|
||
| print("Starting tracker...") | ||
| frames = run_tracker(frames=frames, tracker=tracker) | ||
| tracker.final_pass(frames) | ||
| labels_pr = Labels(labeled_frames=frames) | ||
|
|
||
| labels_pr = Labels(labeled_frames=frames) | ||
| if output_path is None: | ||
| output_path = f"{data_path}.{tracker.get_name()}.slp" | ||
|
|
||
| if output_path is None: | ||
| output_path = f"{data_path}.{tracker.get_name()}.slp" | ||
| if args.no_empty_frames: | ||
| # Clear empty frames if specified. | ||
| labels_pr.remove_empty_frames() | ||
|
|
||
| finish_timestamp = str(datetime.now()) | ||
| total_elapsed = time() - t0 | ||
| print("Finished inference at:", finish_timestamp) | ||
| print(f"Total runtime: {total_elapsed} secs") | ||
| print(f"Predicted frames: {len(labels_pr)}/{len(provider)}") | ||
|
|
||
| # Add provenance metadata to predictions. | ||
| labels_pr.provenance["sleap_version"] = sleap.__version__ | ||
| labels_pr.provenance["platform"] = platform.platform() | ||
| labels_pr.provenance["command"] = " ".join(sys.argv) | ||
| labels_pr.provenance["data_path"] = data_path | ||
| labels_pr.provenance["output_path"] = output_path | ||
| labels_pr.provenance["total_elapsed"] = total_elapsed | ||
| labels_pr.provenance["start_timestamp"] = start_timestamp | ||
| labels_pr.provenance["finish_timestamp"] = finish_timestamp | ||
|
|
||
| print("Provenance:") | ||
| pprint(labels_pr.provenance) | ||
| print() | ||
|
|
||
| labels_pr.provenance["args"] = vars(args) | ||
|
|
||
| # Save results. | ||
| labels_pr.save(output_path) | ||
| print("Saved output:", output_path) | ||
|
|
||
| if args.open_in_gui: | ||
| subprocess.call(["sleap-label", output_path]) | ||
|
|
||
| # Reset output_path for next iteration | ||
| output_path = args.output |
There was a problem hiding this comment.
Review output path handling logic.
The handling of the output path, especially when multiple inputs are given, needs careful attention. Consider refining the logic to ensure that the output path is correctly set in all scenarios.
if output_path is not None and Path.is_file(output_path_obj) and len(data_path_list) > 1:
raise ValueError(
"output_path argument must be a directory if multiple video inputs are given"
)
# Ensure that the output path is reset correctly after each iteration
+ original_output_path = args.output
for data_path, provider in zip(data_path_list, provider_list):
...
- output_path = args.output
+ output_path = original_output_pathCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| provider_list, data_path_list = _make_provider_from_cli(args) | |
| # Setup tracker. | |
| tracker = _make_tracker_from_cli(args) | |
| output_path = args.output | |
| if output_path is not None: | |
| output_path_obj = Path(output_path) | |
| # Output path given is a file, but multiple inputs were given | |
| if output_path is not None and ( | |
| #TODO check if directory exists | |
| #always specify output directory | |
| Path.is_file(output_path_obj) and len(data_path_list) > 1 | |
| ): | |
| raise ValueError( | |
| "output_path argument must be a directory if multiple video inputs are given" | |
| ) | |
| if args.models is not None and "movenet" in args.models[0]: | |
| args.models = args.models[0] | |
| # Either run inference (and tracking) or just run tracking | |
| # Either run inference (and tracking) or just run tracking (if using an existing prediction where inference has already been run) | |
| if args.models is not None: | |
| # Setup models. | |
| predictor = _make_predictor_from_cli(args) | |
| predictor.tracker = tracker | |
| # Run inference! | |
| labels_pr = predictor.predict(provider) | |
| # Run inference on all files inputed | |
| for data_path, provider in zip(data_path_list, provider_list): | |
| # Setup models. | |
| predictor = _make_predictor_from_cli(args) | |
| print(f"predictor.tracker: {tracker}") | |
| predictor.tracker = tracker | |
| # Run inference! | |
| labels_pr = predictor.predict(provider) | |
| if output_path is None: | |
| output_path = data_path + ".predictions.slp" | |
| if output_path is None: | |
| labels_pr.provenance["model_paths"] = predictor.model_paths | |
| labels_pr.provenance["predictor"] = type(predictor).__name__ | |
| output_path = data_path.parent / (data_path.stem + ".predictions.slp") | |
| output_path_obj = Path(output_path) | |
| else: | |
| output_path = output_path + "/" + (data_path.stem + ".predictions.slp") | |
| output_path_obj = Path(output_path) | |
| labels_pr.provenance["model_paths"] = predictor.model_paths | |
| labels_pr.provenance["predictor"] = type(predictor).__name__ | |
| if args.no_empty_frames: | |
| # Clear empty frames if specified. | |
| labels_pr.remove_empty_frames() | |
| finish_timestamp = str(datetime.now()) | |
| total_elapsed = time() - t0 | |
| print("Finished inference at:", finish_timestamp) | |
| print(f"Total runtime: {total_elapsed} secs") | |
| print(f"Predicted frames: {len(labels_pr)}/{len(provider)}") | |
| # Add provenance metadata to predictions. | |
| labels_pr.provenance["sleap_version"] = sleap.__version__ | |
| labels_pr.provenance["platform"] = platform.platform() | |
| labels_pr.provenance["command"] = " ".join(sys.argv) | |
| labels_pr.provenance["data_path"] = data_path.as_posix() | |
| labels_pr.provenance["output_path"] = output_path_obj.as_posix() | |
| labels_pr.provenance["total_elapsed"] = total_elapsed | |
| labels_pr.provenance["start_timestamp"] = start_timestamp | |
| labels_pr.provenance["finish_timestamp"] = finish_timestamp | |
| print("Provenance:") | |
| print(labels_pr.provenance) | |
| print() | |
| labels_pr.provenance["args"] = vars(args) | |
| # Save results. | |
| labels_pr.save(output_path) | |
| print("Saved output:", output_path) | |
| if args.open_in_gui: | |
| subprocess.call(["sleap-label", output_path]) | |
| # Reset output_path for next iteration | |
| output_path = args.output | |
| # running tracking on existing prediction file | |
| elif getattr(args, "tracking.tracker") is not None: | |
| # Load predictions | |
| print("Loading predictions...") | |
| labels_pr = sleap.load_file(args.data_path) | |
| frames = sorted(labels_pr.labeled_frames, key=lambda lf: lf.frame_idx) | |
| for data_path, provider in zip(data_path_list, provider_list): | |
| # Load predictions | |
| print("Loading predictions...") | |
| labels_pr = sleap.load_file(data_path.as_posix()) | |
| frames = sorted(labels_pr.labeled_frames, key=lambda lf: lf.frame_idx) | |
| print("Starting tracker...") | |
| frames = run_tracker(frames=frames, tracker=tracker) | |
| tracker.final_pass(frames) | |
| print("Starting tracker...") | |
| frames = run_tracker(frames=frames, tracker=tracker) | |
| tracker.final_pass(frames) | |
| labels_pr = Labels(labeled_frames=frames) | |
| labels_pr = Labels(labeled_frames=frames) | |
| if output_path is None: | |
| output_path = f"{data_path}.{tracker.get_name()}.slp" | |
| if output_path is None: | |
| output_path = f"{data_path}.{tracker.get_name()}.slp" | |
| if args.no_empty_frames: | |
| # Clear empty frames if specified. | |
| labels_pr.remove_empty_frames() | |
| finish_timestamp = str(datetime.now()) | |
| total_elapsed = time() - t0 | |
| print("Finished inference at:", finish_timestamp) | |
| print(f"Total runtime: {total_elapsed} secs") | |
| print(f"Predicted frames: {len(labels_pr)}/{len(provider)}") | |
| # Add provenance metadata to predictions. | |
| labels_pr.provenance["sleap_version"] = sleap.__version__ | |
| labels_pr.provenance["platform"] = platform.platform() | |
| labels_pr.provenance["command"] = " ".join(sys.argv) | |
| labels_pr.provenance["data_path"] = data_path | |
| labels_pr.provenance["output_path"] = output_path | |
| labels_pr.provenance["total_elapsed"] = total_elapsed | |
| labels_pr.provenance["start_timestamp"] = start_timestamp | |
| labels_pr.provenance["finish_timestamp"] = finish_timestamp | |
| print("Provenance:") | |
| pprint(labels_pr.provenance) | |
| print() | |
| labels_pr.provenance["args"] = vars(args) | |
| # Save results. | |
| labels_pr.save(output_path) | |
| print("Saved output:", output_path) | |
| if args.open_in_gui: | |
| subprocess.call(["sleap-label", output_path]) | |
| # Reset output_path for next iteration | |
| output_path = args.output | |
| provider_list, data_path_list = _make_provider_from_cli(args) | |
| # Setup tracker. | |
| tracker = _make_tracker_from_cli(args) | |
| output_path = args.output | |
| if output_path is not None: | |
| output_path_obj = Path(output_path) | |
| # Output path given is a file, but multiple inputs were given | |
| if output_path is not None and ( | |
| #TODO check if directory exists | |
| #always specify output directory | |
| Path.is_file(output_path_obj) and len(data_path_list) > 1 | |
| ): | |
| raise ValueError( | |
| "output_path argument must be a directory if multiple video inputs are given" | |
| ) | |
| if args.models is not None and "movenet" in args.models[0]: | |
| args.models = args.models[0] | |
| # Either run inference (and tracking) or just run tracking (if using an existing prediction where inference has already been run) | |
| if args.models is not None: | |
| # Run inference on all files inputed | |
| original_output_path = args.output | |
| for data_path, provider in zip(data_path_list, provider_list): | |
| # Setup models. | |
| predictor = _make_predictor_from_cli(args) | |
| print(f"predictor.tracker: {tracker}") | |
| predictor.tracker = tracker | |
| # Run inference! | |
| labels_pr = predictor.predict(provider) | |
| if output_path is None: | |
| output_path = data_path.parent / (data_path.stem + ".predictions.slp") | |
| output_path_obj = Path(output_path) | |
| else: | |
| output_path = output_path + "/" + (data_path.stem + ".predictions.slp") | |
| output_path_obj = Path(output_path) | |
| labels_pr.provenance["model_paths"] = predictor.model_paths | |
| labels_pr.provenance["predictor"] = type(predictor).__name__ | |
| if args.no_empty_frames: | |
| # Clear empty frames if specified. | |
| labels_pr.remove_empty_frames() | |
| finish_timestamp = str(datetime.now()) | |
| total_elapsed = time() - t0 | |
| print("Finished inference at:", finish_timestamp) | |
| print(f"Total runtime: {total_elapsed} secs") | |
| print(f"Predicted frames: {len(labels_pr)}/{len(provider)}") | |
| # Add provenance metadata to predictions. | |
| labels_pr.provenance["sleap_version"] = sleap.__version__ | |
| labels_pr.provenance["platform"] = platform.platform() | |
| labels_pr.provenance["command"] = " ".join(sys.argv) | |
| labels_pr.provenance["data_path"] = data_path.as_posix() | |
| labels_pr.provenance["output_path"] = output_path_obj.as_posix() | |
| labels_pr.provenance["total_elapsed"] = total_elapsed | |
| labels_pr.provenance["start_timestamp"] = start_timestamp | |
| labels_pr.provenance["finish_timestamp"] = finish_timestamp | |
| print("Provenance:") | |
| print(labels_pr.provenance) | |
| print() | |
| labels_pr.provenance["args"] = vars(args) | |
| # Save results. | |
| labels_pr.save(output_path) | |
| print("Saved output:", output_path) | |
| if args.open_in_gui: | |
| subprocess.call(["sleap-label", output_path]) | |
| # Reset output_path for next iteration | |
| output_path = original_output_path | |
| # running tracking on existing prediction file | |
| elif getattr(args, "tracking.tracker") is not None: | |
| for data_path, provider in zip(data_path_list, provider_list): | |
| # Load predictions | |
| print("Loading predictions...") | |
| labels_pr = sleap.load_file(data_path.as_posix()) | |
| frames = sorted(labels_pr.labeled_frames, key=lambda lf: lf.frame_idx) | |
| print("Starting tracker...") | |
| frames = run_tracker(frames=frames, tracker=tracker) | |
| tracker.final_pass(frames) | |
| labels_pr = Labels(labeled_frames=frames) | |
| if output_path is None: | |
| output_path = f"{data_path}.{tracker.get_name()}.slp" | |
| if args.no_empty_frames: | |
| # Clear empty frames if specified. | |
| labels_pr.remove_empty_frames() | |
| finish_timestamp = str(datetime.now()) | |
| total_elapsed = time() - t0 | |
| print("Finished inference at:", finish_timestamp) | |
| print(f"Total runtime: {total_elapsed} secs") | |
| print(f"Predicted frames: {len(labels_pr)}/{len(provider)}") | |
| # Add provenance metadata to predictions. | |
| labels_pr.provenance["sleap_version"] = sleap.__version__ | |
| labels_pr.provenance["platform"] = platform.platform() | |
| labels_pr.provenance["command"] = " ".join(sys.argv) | |
| labels_pr.provenance["data_path"] = data_path | |
| labels_pr.provenance["output_path"] = output_path | |
| labels_pr.provenance["total_elapsed"] = total_elapsed | |
| labels_pr.provenance["start_timestamp"] = start_timestamp | |
| labels_pr.provenance["finish_timestamp"] = finish_timestamp | |
| print("Provenance:") | |
| pprint(labels_pr.provenance) | |
| print() | |
| labels_pr.provenance["args"] = vars(args) | |
| # Save results. | |
| labels_pr.save(output_path) | |
| print("Saved output:", output_path) | |
| if args.open_in_gui: | |
| subprocess.call(["sleap-label", output_path]) | |
| # Reset output_path for next iteration | |
| output_path = args.output |
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (4)
sleap/nn/inference.py (4)
5291-5298: Optimize conditional assignment using a ternary operator.The current
if-elseblock for settingdata_pathcan be simplified using a ternary operator, which makes the code cleaner and more concise.- if labels_path is not None: - data_path = Path(labels_path) - else: - data_path = Path(args.data_path) + data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)Tools
Ruff
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-blockReplace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)(SIM108)
5307-5307: Ensure consistent path format.Use
as_posix()consistently to ensure the path format is consistent across different operating systems.- data_path_list = [data_path.as_posix()] + data_path_list = [data_path.as_posix() for data_path in data_path_list]
5373-5373: Use logging instead of print statements.Using logging instead of print statements provides better control over the output and can be easily disabled or redirected.
- print(args) + logging.debug(args)
5527-5528: Clarify error message.The error message can be clarified to provide more information to the user.
- "output_path argument must be a directory if multiple video inputs are given" + "The output_path argument must be a directory when multiple video inputs are given. Please specify a valid directory."
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (5)
tests/data/videos/multiple_inputs_mp4/centered_pair_small.mp4is excluded by!**/*.mp4tests/data/videos/multiple_inputs_mp4/centered_pair_small_2.mp4is excluded by!**/*.mp4tests/data/videos/multiple_inputs_mp4/centered_pair_small_3.mp4is excluded by!**/*.mp4tests/data/videos/multiple_inputs_slp_mp4/centered_pair_small_2.mp4is excluded by!**/*.mp4tests/data/videos/multiple_inputs_slp_mp4/centered_pair_small_3.mp4is excluded by!**/*.mp4
Files selected for processing (1)
- sleap/nn/inference.py (5 hunks)
Additional context used
Ruff
sleap/nn/inference.py
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-blockReplace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)(SIM108)
Additional comments not posted (1)
sleap/nn/inference.py (1)
5608-5612: Ensure directory creation for output path.Ensure that the directory for the output path is created if it does not exist.
output_path_obj.mkdir(parents=True, exist_ok=True)
sleap/nn/inference.py
Outdated
| output_path = output_path + "/" + (data_path_obj.stem + ".predictions.slp") | ||
| output_path_obj = Path(output_path) | ||
|
|
There was a problem hiding this comment.
Ensure directory creation for output path.
Ensure that the directory for the output path is created if it does not exist.
output_path_obj.mkdir(parents=True, exist_ok=True)
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/nn/inference.py (5 hunks)
Additional context used
Ruff
sleap/nn/inference.py
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-blockReplace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)(SIM108)
sleap/nn/inference.py
Outdated
| if data_path.is_dir(): | ||
| data_path_list = [] | ||
| for file_path in data_path.iterdir(): | ||
| if file_path.is_file(): | ||
| data_path_list.append(file_path) | ||
| elif data_path.is_file(): |
There was a problem hiding this comment.
Add error handling for empty directories.
Currently, if the directory is empty, data_path_list will be empty, which might lead to unexpected behavior later in the code. Consider adding a check to handle this case.
if not data_path_list:
raise ValueError("The specified directory is empty.")
sleap/nn/inference.py
Outdated
| provider_list = [] | ||
| tmp_data_path_list = [] | ||
| for data_path in data_path_list: | ||
| # Create a provider for each file | ||
| data_path_obj = Path(data_path) | ||
| if data_path_obj.as_posix().endswith(".slp"): | ||
| print(f"Sleap file: {data_path_obj}") | ||
| labels = sleap.load_file(data_path_obj.as_posix()) | ||
|
|
||
| if args.only_labeled_frames: | ||
| provider_list.append(LabelsReader.from_user_labeled_frames(labels)) | ||
| elif args.only_suggested_frames: | ||
| provider_list.append(LabelsReader.from_unlabeled_suggestions(labels)) | ||
| elif getattr(args, "video.index") != "": | ||
| provider_list.append( | ||
| VideoReader( | ||
| video=labels.videos[int(getattr(args, "video.index"))], | ||
| example_indices=frame_list(args.frames), | ||
| ) | ||
| ) | ||
| else: | ||
| provider_list.append(LabelsReader(labels)) | ||
|
|
||
| tmp_data_path_list.append(data_path_obj) | ||
|
|
||
| else: | ||
| provider = LabelsReader(labels) | ||
| try: | ||
| video_kwargs = dict( | ||
| dataset=vars(args).get("video.dataset"), | ||
| input_format=vars(args).get("video.input_format"), | ||
| ) | ||
| provider_list.append( | ||
| VideoReader.from_filepath( | ||
| filename=data_path.as_posix(), | ||
| example_indices=frame_list(args.frames), | ||
| **video_kwargs, | ||
| ) | ||
| ) | ||
| print(f"Video: {data_path}") | ||
| tmp_data_path_list.append(data_path) | ||
| # TODO: Clean this up. | ||
| except Exception: | ||
| print(f"Error reading file: {data_path}") | ||
|
|
||
| else: | ||
| print(f"Video: {data_path}") | ||
| # TODO: Clean this up. | ||
| video_kwargs = dict( | ||
| dataset=vars(args).get("video.dataset"), | ||
| input_format=vars(args).get("video.input_format"), | ||
| ) | ||
| provider = VideoReader.from_filepath( | ||
| filename=data_path, example_indices=frame_list(args.frames), **video_kwargs | ||
| ) | ||
| data_path_list = tmp_data_path_list | ||
|
|
||
| return provider, data_path | ||
| return provider_list, data_path_list |
There was a problem hiding this comment.
Refactor to improve readability and maintainability.
The nested loops and conditions can be refactored to improve readability and maintainability. Consider extracting the logic for creating providers into a separate function.
def create_provider(data_path, args):
data_path_obj = Path(data_path)
if data_path_obj.as_posix().endswith(".slp"):
labels = sleap.load_file(data_path_obj.as_posix())
if args.only_labeled_frames:
return LabelsReader.from_user_labeled_frames(labels)
elif args.only_suggested_frames:
return LabelsReader.from_unlabeled_suggestions(labels)
elif getattr(args, "video.index") != "":
return VideoReader(
video=labels.videos[int(getattr(args, "video.index"))],
example_indices=frame_list(args.frames),
)
else:
return LabelsReader(labels)
else:
video_kwargs = dict(
dataset=vars(args).get("video.dataset"),
input_format=vars(args).get("video.input_format"),
)
return VideoReader.from_filepath(
filename=data_path.as_posix(),
example_indices=frame_list(args.frames),
**video_kwargs,
)
provider_list = [create_provider(data_path, args) for data_path in data_path_list]
sleap/nn/inference.py
Outdated
|
|
||
| # Figure out which input path to use. | ||
| labels_path = getattr(args, "labels", None) | ||
| if labels_path is not None: | ||
| data_path = labels_path | ||
| data_path = Path(labels_path) | ||
| else: | ||
| data_path = Path(args.data_path) | ||
|
|
There was a problem hiding this comment.
Optimize conditional assignment using a ternary operator.
The current if-else block for setting data_path can be simplified using a ternary operator, which makes the code cleaner and more concise.
- if labels_path is not None:
- data_path = Path(labels_path)
- else:
- data_path = Path(args.data_path)
+ data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Figure out which input path to use. | |
| labels_path = getattr(args, "labels", None) | |
| if labels_path is not None: | |
| data_path = labels_path | |
| data_path = Path(labels_path) | |
| else: | |
| data_path = Path(args.data_path) | |
| # Figure out which input path to use. | |
| labels_path = getattr(args, "labels", None) | |
| data_path = Path(labels_path) if labels_path is not None else Path(args.data_path) | |
Tools
Ruff
5294-5297: Use ternary operator
data_path = Path(labels_path) if labels_path is not None else Path(args.data_path)instead ofif-else-blockReplace
if-else-block withdata_path = Path(labels_path) if labels_path is not None else Path(args.data_path)(SIM108)
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (2)
sleap/nn/inference.py (2)
5372-5372: Consider using logging instead of print.Using
loggingmodule.- print(args) + logging.debug(args)
5453-5453: Enhance logging for better traceability.Consider adding more detailed logging when parsing arguments and setting up devices. This can help in debugging and understanding the flow of execution.
logging.info("Parsed arguments: %s", vars(args)) if args.cpu or not sleap.nn.system.is_gpu_system(): logging.info("No GPU found, using CPU only.") sleap.nn.system.use_cpu_only()
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/nn/inference.py (5 hunks)
Additional context used
Ruff
sleap/nn/inference.py
5295-5295: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
| subprocess.call(["sleap-label", output_path]) | ||
|
|
||
| # Reset output_path for next iteration | ||
| output_path = args.output |
There was a problem hiding this comment.
Add output folder as an option when the input is a folder.
This suggestion addresses the need to specify an output folder when processing multiple video files from a directory.
Do you want me to generate the code to implement this feature or open a GitHub issue to track this task?
| # Reset output_path for next iteration | ||
| output_path = args.output |
There was a problem hiding this comment.
Resetting output_path might lead to unintended behavior.
The reset of output_path at the end of each iteration could lead to unexpected behavior in subsequent iterations, especially if args.output is modified during the process.
+ original_output_path = args.output
for data_path, provider in zip(data_path_list, provider_list):
...
- output_path = args.output
+ output_path = original_output_pathCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Reset output_path for next iteration | |
| output_path = args.output | |
| original_output_path = args.output | |
| for data_path, provider in zip(data_path_list, provider_list): | |
| ... | |
| output_path = original_output_path |
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range, codebase verification and nitpick comments (1)
tests/nn/test_inference.py (1)
Line range hint
1511-1538: Add assertions after the invalid command test.The function
test_sleap_track_single_inputis well-structured but lacks assertions after the invalid command test to ensure the expected behavior.# Create invalid sleap-track command args = [slp_path, "--cpu"] with pytest.raises(ValueError): sleap_track(args=args) + # Add assertions to verify the expected behavior + assert not Path(f"{slp_path}.predictions.slp").exists()
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (5 hunks)
- tests/nn/test_inference.py (3 hunks)
Additional context used
Ruff
sleap/nn/inference.py
5295-5295: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
Additional comments not posted (5)
tests/nn/test_inference.py (4)
1480-1509: LGTM! The function is well-structured and comprehensive.The function
test_make_predictor_from_cli_mult_inputeffectively tests the creation of predictors from multiple input sources and follows a clear and logical structure.
1717-1736: LGTM! The function correctly raises an exception for invalid output paths.The function
test_sleap_track_invalid_outputis well-structured and correctly tests the behavior for invalid output paths.
1739-1755: LGTM! The function correctly raises an exception for invalid input paths.The function
test_sleap_track_invalid_inputis well-structured and correctly tests the behavior for invalid input paths.
1758-1780: LGTM! The function is well-structured and comprehensive.The function
test_sleap_track_user_labeled_frameseffectively tests thesleap-trackcommand with only labeled frames and follows a clear and logical structure.sleap/nn/inference.py (1)
5372-5372: Consider using logging instead of print.Using
loggingmodule.- print(args) + logging.debug(args)Likely invalid or redundant comment.
| try: | ||
| video_kwargs = dict( | ||
| dataset=vars(args).get("video.dataset"), | ||
| input_format=vars(args).get("video.input_format"), | ||
| ) | ||
| output_provider_list.append( | ||
| VideoReader.from_filepath( | ||
| filename=file_path.as_posix(), | ||
| example_indices=frame_list(args.frames), | ||
| **video_kwargs, | ||
| ) | ||
| ) | ||
| print(f"Video: {file_path.as_posix()}") | ||
| output_data_path_list.append(file_path) | ||
| # TODO: Clean this up. | ||
| except Exception: | ||
| print(f"Error reading file: {file_path.as_posix()}") |
There was a problem hiding this comment.
Handle exceptions when reading files.
The code should handle exceptions more gracefully when reading files to avoid partial processing.
+ import logging
for file_path in data_path_list:
try:
# Create a provider for each file
if file_path.as_posix().endswith(".slp"):
labels = sleap.load_file(file_path.as_posix())
...
else:
video_kwargs = dict(
dataset=vars(args).get("video.dataset"),
input_format=vars(args).get("video.input_format"),
)
output_provider_list.append(
VideoReader.from_filepath(
filename=file_path.as_posix(),
example_indices=frame_list(args.frames),
**video_kwargs,
)
)
print(f"Video: {file_path.as_posix()}")
output_data_path_list.append(file_path)
except Exception as e:
logging.error(f"Error reading file: {file_path.as_posix()} - {e}")Committable suggestion was skipped due to low confidence.
tests/nn/test_inference.py
Outdated
| @pytest.mark.parametrize("tracking", ["simple", "flow", "None"]) | ||
| def test_sleap_track_mult_input_mp4( | ||
| min_centroid_model_path: str, | ||
| min_centered_instance_model_path: str, | ||
| test_sleap_track_mult_inputs_folder_mp4: str, | ||
| tracking, | ||
| ): | ||
| slp_path = test_sleap_track_mult_inputs_folder_mp4 | ||
| slp_path_obj = Path(slp_path) | ||
|
|
||
| # Create sleap-track command | ||
| args = ( | ||
| f"{slp_path} --model {min_centroid_model_path} " | ||
| f"--tracking.tracker {tracking} " | ||
| f"--model {min_centered_instance_model_path} --video.index 0 --frames 1-3 --cpu" | ||
| ).split() | ||
|
|
||
| slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | ||
|
|
||
| # Run inference | ||
| sleap_track(args=args) | ||
|
|
||
| # Assert predictions file exists | ||
| expected_extensions = { | ||
| ".slp", | ||
| ".mp4", | ||
| ".avi", | ||
| } # Add other video formats if necessary | ||
|
|
||
| for file_path in slp_path_list: | ||
| if file_path.suffix in expected_extensions: | ||
| expected_output_file = f"{file_path}.predictions.slp" | ||
| assert Path(expected_output_file).exists() | ||
|
|
||
| new_slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | ||
|
|
||
| # remove the files that have been added within the test case to revert input folders to their original state | ||
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | ||
| for file in files_to_remove: | ||
| file.unlink() |
There was a problem hiding this comment.
Add comments explaining the removal of files.
The function test_sleap_track_mult_input_mp4 is well-structured but lacks comments explaining why files are removed.
# remove the files that have been added within the test case to revert input folders to their original state
+ # This ensures the input folder is reverted to its original state after the test.
files_to_remove = set(new_slp_path_list) - set(slp_path_list)
for file in files_to_remove:
file.unlink()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.parametrize("tracking", ["simple", "flow", "None"]) | |
| def test_sleap_track_mult_input_mp4( | |
| min_centroid_model_path: str, | |
| min_centered_instance_model_path: str, | |
| test_sleap_track_mult_inputs_folder_mp4: str, | |
| tracking, | |
| ): | |
| slp_path = test_sleap_track_mult_inputs_folder_mp4 | |
| slp_path_obj = Path(slp_path) | |
| # Create sleap-track command | |
| args = ( | |
| f"{slp_path} --model {min_centroid_model_path} " | |
| f"--tracking.tracker {tracking} " | |
| f"--model {min_centered_instance_model_path} --video.index 0 --frames 1-3 --cpu" | |
| ).split() | |
| slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | |
| # Run inference | |
| sleap_track(args=args) | |
| # Assert predictions file exists | |
| expected_extensions = { | |
| ".slp", | |
| ".mp4", | |
| ".avi", | |
| } # Add other video formats if necessary | |
| for file_path in slp_path_list: | |
| if file_path.suffix in expected_extensions: | |
| expected_output_file = f"{file_path}.predictions.slp" | |
| assert Path(expected_output_file).exists() | |
| new_slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | |
| # remove the files that have been added within the test case to revert input folders to their original state | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() | |
| # remove the files that have been added within the test case to revert input folders to their original state | |
| # This ensures the input folder is reverted to its original state after the test. | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() |
tests/nn/test_inference.py
Outdated
| def test_sleap_track_output_mult( | ||
| test_sleap_track_output_folder: str, | ||
| min_centroid_model_path: str, | ||
| min_centered_instance_model_path: str, | ||
| test_sleap_track_mult_inputs_folder_mp4: str, | ||
| ): | ||
| slp_path = test_sleap_track_mult_inputs_folder_mp4 | ||
| slp_path_obj = Path(slp_path) | ||
| output_path = test_sleap_track_output_folder | ||
| output_path_obj = Path(output_path) | ||
|
|
||
| # Create sleap-track command | ||
| args = ( | ||
| f"{slp_path} --model {min_centroid_model_path} " | ||
| f"--tracking.tracker simple " | ||
| f"-o {output_path} " | ||
| f"--model {min_centered_instance_model_path} --video.index 0 --frames 1-3 --cpu" | ||
| ).split() | ||
|
|
||
| slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | ||
|
|
||
| output_path_list = [file for file in output_path_obj.iterdir() if file.is_file()] | ||
|
|
||
| # Run inference | ||
| sleap_track(args=args) | ||
| slp_path = Path(slp_path) | ||
|
|
||
| # Assert predictions file exists | ||
| new_output_path_list = [ | ||
| file for file in output_path_obj.iterdir() if file.is_file() | ||
| ] | ||
|
|
||
| # Check if there are any files in the directory | ||
| expected_extensions = { | ||
| ".slp", | ||
| ".mp4", | ||
| ".avi", | ||
| } # Add other video formats if necessary | ||
|
|
||
| for file_path in slp_path_list: | ||
| if file_path.suffix in expected_extensions: | ||
| expected_output_file = Path(output_path) / ( | ||
| file_path.stem + ".predictions.slp" | ||
| ) | ||
| assert Path(expected_output_file).exists() | ||
|
|
||
| # remove the files that have been added within the test case to revert input folders to their original state | ||
| files_to_remove = set(new_output_path_list) - set(output_path_list) | ||
| for file in files_to_remove: | ||
| file.unlink() |
There was a problem hiding this comment.
Add comments explaining the removal of files.
The function test_sleap_track_output_mult is well-structured but lacks comments explaining why files are removed.
# remove the files that have been added within the test case to revert input folders to their original state
+ # This ensures the output folder is reverted to its original state after the test.
files_to_remove = set(new_output_path_list) - set(output_path_list)
for file in files_to_remove:
file.unlink()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_sleap_track_output_mult( | |
| test_sleap_track_output_folder: str, | |
| min_centroid_model_path: str, | |
| min_centered_instance_model_path: str, | |
| test_sleap_track_mult_inputs_folder_mp4: str, | |
| ): | |
| slp_path = test_sleap_track_mult_inputs_folder_mp4 | |
| slp_path_obj = Path(slp_path) | |
| output_path = test_sleap_track_output_folder | |
| output_path_obj = Path(output_path) | |
| # Create sleap-track command | |
| args = ( | |
| f"{slp_path} --model {min_centroid_model_path} " | |
| f"--tracking.tracker simple " | |
| f"-o {output_path} " | |
| f"--model {min_centered_instance_model_path} --video.index 0 --frames 1-3 --cpu" | |
| ).split() | |
| slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | |
| output_path_list = [file for file in output_path_obj.iterdir() if file.is_file()] | |
| # Run inference | |
| sleap_track(args=args) | |
| slp_path = Path(slp_path) | |
| # Assert predictions file exists | |
| new_output_path_list = [ | |
| file for file in output_path_obj.iterdir() if file.is_file() | |
| ] | |
| # Check if there are any files in the directory | |
| expected_extensions = { | |
| ".slp", | |
| ".mp4", | |
| ".avi", | |
| } # Add other video formats if necessary | |
| for file_path in slp_path_list: | |
| if file_path.suffix in expected_extensions: | |
| expected_output_file = Path(output_path) / ( | |
| file_path.stem + ".predictions.slp" | |
| ) | |
| assert Path(expected_output_file).exists() | |
| # remove the files that have been added within the test case to revert input folders to their original state | |
| files_to_remove = set(new_output_path_list) - set(output_path_list) | |
| for file in files_to_remove: | |
| file.unlink() | |
| # remove the files that have been added within the test case to revert input folders to their original state | |
| # This ensures the output folder is reverted to its original state after the test. | |
| files_to_remove = set(new_output_path_list) - set(output_path_list) | |
| for file in files_to_remove: | |
| file.unlink() |
tests/nn/test_inference.py
Outdated
| @pytest.mark.parametrize("tracking", ["simple", "flow", "None"]) | ||
| def test_sleap_track_mult_input_slp_mp4( | ||
| min_centroid_model_path: str, | ||
| min_centered_instance_model_path: str, | ||
| test_sleap_track_mult_inputs_folder_slp_mp4: str, | ||
| tracking, | ||
| ): | ||
| slp_path = test_sleap_track_mult_inputs_folder_slp_mp4 | ||
| slp_path_obj = Path(slp_path) | ||
|
|
||
| # Create sleap-track command | ||
| args = ( | ||
| f"{slp_path} --model {min_centroid_model_path} " | ||
| f"--tracking.tracker {tracking} " | ||
| f"--model {min_centered_instance_model_path} --video.index 0 --frames 1-3 --cpu" | ||
| ).split() | ||
|
|
||
| slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | ||
|
|
||
| # Run inference | ||
| sleap_track(args=args) | ||
|
|
||
| # Assert predictions file exists | ||
| expected_extensions = { | ||
| ".slp", | ||
| ".mp4", | ||
| ".avi", | ||
| } # Add other video formats if necessary | ||
|
|
||
| for file_path in slp_path_list: | ||
| if file_path.suffix in expected_extensions: | ||
| expected_output_file = f"{file_path}.predictions.slp" | ||
| assert Path(expected_output_file).exists() | ||
|
|
||
| new_slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | ||
|
|
||
| # remove the files that have been added within the test case to revert input folders to their original state | ||
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | ||
| for file in files_to_remove: | ||
| file.unlink() |
There was a problem hiding this comment.
Add comments explaining the removal of files.
The function test_sleap_track_mult_input_slp_mp4 is well-structured but lacks comments explaining why files are removed.
# remove the files that have been added within the test case to revert input folders to their original state
+ # This ensures the input folder is reverted to its original state after the test.
files_to_remove = set(new_slp_path_list) - set(slp_path_list)
for file in files_to_remove:
file.unlink()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.parametrize("tracking", ["simple", "flow", "None"]) | |
| def test_sleap_track_mult_input_slp_mp4( | |
| min_centroid_model_path: str, | |
| min_centered_instance_model_path: str, | |
| test_sleap_track_mult_inputs_folder_slp_mp4: str, | |
| tracking, | |
| ): | |
| slp_path = test_sleap_track_mult_inputs_folder_slp_mp4 | |
| slp_path_obj = Path(slp_path) | |
| # Create sleap-track command | |
| args = ( | |
| f"{slp_path} --model {min_centroid_model_path} " | |
| f"--tracking.tracker {tracking} " | |
| f"--model {min_centered_instance_model_path} --video.index 0 --frames 1-3 --cpu" | |
| ).split() | |
| slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | |
| # Run inference | |
| sleap_track(args=args) | |
| # Assert predictions file exists | |
| expected_extensions = { | |
| ".slp", | |
| ".mp4", | |
| ".avi", | |
| } # Add other video formats if necessary | |
| for file_path in slp_path_list: | |
| if file_path.suffix in expected_extensions: | |
| expected_output_file = f"{file_path}.predictions.slp" | |
| assert Path(expected_output_file).exists() | |
| new_slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | |
| # remove the files that have been added within the test case to revert input folders to their original state | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() | |
| # remove the files that have been added within the test case to revert input folders to their original state | |
| # This ensures the input folder is reverted to its original state after the test. | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() |
tests/nn/test_inference.py
Outdated
| @pytest.mark.parametrize("tracking", ["simple", "flow", "None"]) | ||
| def test_sleap_track_mult_input_slp( | ||
| min_centroid_model_path: str, | ||
| min_centered_instance_model_path: str, | ||
| test_sleap_track_mult_inputs_folder_slp: str, | ||
| tracking, | ||
| ): | ||
| slp_path = test_sleap_track_mult_inputs_folder_slp | ||
| slp_path_obj = Path(slp_path) | ||
|
|
||
| # Create sleap-track command | ||
| args = ( | ||
| f"{slp_path} --model {min_centroid_model_path} " | ||
| f"--tracking.tracker {tracking} " | ||
| f"--model {min_centered_instance_model_path} --video.index 0 --frames 1-3 --cpu" | ||
| ).split() | ||
|
|
||
| slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | ||
|
|
||
| # Run inference | ||
| sleap_track(args=args) | ||
|
|
||
| # Assert predictions file exists | ||
| expected_extensions = { | ||
| ".slp", | ||
| ".mp4", | ||
| ".avi", | ||
| } # Add other video formats if necessary | ||
|
|
||
| for file_path in slp_path_list: | ||
| if file_path.suffix in expected_extensions: | ||
| expected_output_file = f"{slp_path}.predictions.slp" | ||
| assert Path(expected_output_file).exists() | ||
|
|
||
| new_slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | ||
|
|
||
| # remove the files that have been added within the test case to revert input folders to their original state | ||
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | ||
| for file in files_to_remove: | ||
| file.unlink() |
There was a problem hiding this comment.
Add comments explaining the removal of files.
The function test_sleap_track_mult_input_slp is well-structured but lacks comments explaining why files are removed.
# remove the files that have been added within the test case to revert input folders to their original state
+ # This ensures the input folder is reverted to its original state after the test.
files_to_remove = set(new_slp_path_list) - set(slp_path_list)
for file in files_to_remove:
file.unlink()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.parametrize("tracking", ["simple", "flow", "None"]) | |
| def test_sleap_track_mult_input_slp( | |
| min_centroid_model_path: str, | |
| min_centered_instance_model_path: str, | |
| test_sleap_track_mult_inputs_folder_slp: str, | |
| tracking, | |
| ): | |
| slp_path = test_sleap_track_mult_inputs_folder_slp | |
| slp_path_obj = Path(slp_path) | |
| # Create sleap-track command | |
| args = ( | |
| f"{slp_path} --model {min_centroid_model_path} " | |
| f"--tracking.tracker {tracking} " | |
| f"--model {min_centered_instance_model_path} --video.index 0 --frames 1-3 --cpu" | |
| ).split() | |
| slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | |
| # Run inference | |
| sleap_track(args=args) | |
| # Assert predictions file exists | |
| expected_extensions = { | |
| ".slp", | |
| ".mp4", | |
| ".avi", | |
| } # Add other video formats if necessary | |
| for file_path in slp_path_list: | |
| if file_path.suffix in expected_extensions: | |
| expected_output_file = f"{slp_path}.predictions.slp" | |
| assert Path(expected_output_file).exists() | |
| new_slp_path_list = [file for file in slp_path_obj.iterdir() if file.is_file()] | |
| # remove the files that have been added within the test case to revert input folders to their original state | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() | |
| # remove the files that have been added within the test case to revert input folders to their original state | |
| # This ensures the input folder is reverted to its original state after the test. | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() |
|
Review notes from subgroup:
|
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (5 hunks)
- tests/nn/test_inference.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/nn/inference.py
Additional comments not posted (7)
tests/nn/test_inference.py (7)
Line range hint
1511-1538:
LGTM!The function is well-structured and tests the
sleap-trackcommand with a single input.
1717-1736: LGTM!The function is well-structured and tests the
sleap-trackcommand with an invalid output file.
1739-1755: LGTM!The function is well-structured and tests the
sleap-trackcommand with an invalid input.
1487-1487: UsePath(test_sleap_track_mult_inputs_folder_slp).as_posix()instead ofstr(...).This ensures consistency in handling file paths.
- slp_path = Path(test_sleap_track_mult_inputs_folder_slp).as_posix() + slp_path = str(Path(test_sleap_track_mult_inputs_folder_slp))Likely invalid or redundant comment.
1692-1710: Add comments explaining the removal of files.The function
test_sleap_track_output_multis well-structured but lacks comments explaining why files are removed.- files_to_remove = set(new_output_path_list) - set(output_path_list) + # This ensures the output folder is reverted to its original state after the test. + files_to_remove = set(new_output_path_list) - set(output_path_list)Likely invalid or redundant comment.
1575-1578: Add comments explaining the removal of files.The function
test_sleap_track_mult_input_slpis well-structured but lacks comments explaining why files are removed.- files_to_remove = set(new_slp_path_list) - set(slp_path_list) + # This ensures the input folder is reverted to its original state after the test. + files_to_remove = set(new_slp_path_list) - set(slp_path_list)Likely invalid or redundant comment.
1487-1487: UsePath(test_sleap_track_mult_inputs_folder_slp).as_posix()instead ofstr(...).This ensures consistency in handling file paths.
- slp_path = Path(test_sleap_track_mult_inputs_folder_slp).as_posix() + slp_path = str(Path(test_sleap_track_mult_inputs_folder_slp))Likely invalid or redundant comment.
tests/nn/test_inference.py
Outdated
| files_to_remove = set(new_output_path_list) - set(output_path_list) | ||
| for file in files_to_remove: | ||
| file.unlink() |
There was a problem hiding this comment.
Add comments explaining the removal of files.
The function test_sleap_track_output_mult is well-structured but lacks comments explaining why files are removed.
- files_to_remove = set(new_output_path_list) - set(output_path_list)
+ # This ensures the output folder is reverted to its original state after the test.
+ files_to_remove = set(new_output_path_list) - set(output_path_list)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| files_to_remove = set(new_output_path_list) - set(output_path_list) | |
| for file in files_to_remove: | |
| file.unlink() | |
| # This ensures the output folder is reverted to its original state after the test. | |
| files_to_remove = set(new_output_path_list) - set(output_path_list) | |
| for file in files_to_remove: | |
| file.unlink() |
tests/nn/test_inference.py
Outdated
| # remove the files that have been added within the test case to revert input folders to their original state | ||
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | ||
| for file in files_to_remove: | ||
| file.unlink() |
There was a problem hiding this comment.
Add comments explaining the removal of files.
The function test_sleap_track_mult_input_slp_mp4 is well-structured but lacks comments explaining why files are removed.
- files_to_remove = set(new_slp_path_list) - set(slp_path_list)
+ # This ensures the input folder is reverted to its original state after the test.
+ files_to_remove = set(new_slp_path_list) - set(slp_path_list)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # remove the files that have been added within the test case to revert input folders to their original state | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() | |
| # remove the files that have been added within the test case to revert input folders to their original state | |
| # This ensures the input folder is reverted to its original state after the test. | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() |
tests/nn/test_inference.py
Outdated
| # remove the files that have been added within the test case to revert input folders to their original state | ||
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | ||
| for file in files_to_remove: | ||
| file.unlink() |
There was a problem hiding this comment.
Add comments explaining the removal of files.
The function test_sleap_track_mult_input_slp is well-structured but lacks comments explaining why files are removed.
- files_to_remove = set(new_slp_path_list) - set(slp_path_list)
+ # This ensures the input folder is reverted to its original state after the test.
+ files_to_remove = set(new_slp_path_list) - set(slp_path_list)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # remove the files that have been added within the test case to revert input folders to their original state | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() | |
| # remove the files that have been added within the test case to revert input folders to their original state | |
| # This ensures the input folder is reverted to its original state after the test. | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() |
tests/nn/test_inference.py
Outdated
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | ||
| for file in files_to_remove: | ||
| file.unlink() |
There was a problem hiding this comment.
Add comments explaining the removal of files.
The function test_sleap_track_mult_input_mp4 is well-structured but lacks comments explaining why files are removed.
- files_to_remove = set(new_slp_path_list) - set(slp_path_list)
+ # This ensures the input folder is reverted to its original state after the test.
+ files_to_remove = set(new_slp_path_list) - set(slp_path_list)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() | |
| # This ensures the input folder is reverted to its original state after the test. | |
| files_to_remove = set(new_slp_path_list) - set(slp_path_list) | |
| for file in files_to_remove: | |
| file.unlink() |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (5 hunks)
- tests/nn/test_inference.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/nn/inference.py
Additional comments not posted (9)
tests/nn/test_inference.py (9)
1451-1480: LGTM!The function
test_make_predictor_from_cli_mult_inputcorrectly tests the creation of a predictor from CLI arguments with multiple inputs.
Line range hint
1482-1509: LGTM!The function
test_sleap_track_single_inputcorrectly tests thesleap-trackcommand with a single input, including both valid and invalid scenarios.
1511-1558: LGTM!The function
test_sleap_track_mult_input_slpcorrectly tests thesleap-trackcommand with multiple SLP files as input, ensuring predictions are generated for each file.
1561-1607: LGTM!The function
test_sleap_track_mult_input_slp_mp4correctly tests thesleap-trackcommand with a mix of SLP and MP4 files as input, ensuring predictions are generated for each file.
1610-1653: LGTM!The function
test_sleap_track_mult_input_mp4correctly tests thesleap-trackcommand with multiple MP4 files as input, ensuring predictions are generated for each file.
1655-1704: LGTM!The function
test_sleap_track_output_multcorrectly tests thesleap-trackcommand with multiple outputs, ensuring predictions are saved to the specified output directory.
1706-1737: LGTM!The function
test_sleap_track_invalid_outputcorrectly tests thesleap-trackcommand with an invalid output path, ensuring aValueErroris raised.
1740-1757: LGTM!The function
test_sleap_track_invalid_inputcorrectly tests thesleap-trackcommand with an invalid input path, ensuring aValueErroris raised.
1560-1608: LGTM!The function
test_sleap_track_mult_inputs_folder_mp4correctly tests thesleap-trackcommand with multiple MP4 files as input, ensuring predictions are generated and saved for each file.
|
In addition to the changes above, another comment re: supporting inputs that are multiple SLP files:
@roomrys am I missing any other dangerzone cases? |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (5 hunks)
- tests/nn/test_inference.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/nn/inference.py
Additional context used
Ruff
tests/nn/test_inference.py
1726-1726: Undefined name
as_posix(F821)
Additional comments not posted (10)
tests/nn/test_inference.py (10)
1451-1491: Ensure proper handling of multiple input files.The function
test_make_predictor_from_cli_mult_inputcorrectly handles multiple input SLP files by copying them into a temporary directory and running inference on each copy. The assertions ensure that the predictor correctly handles the maximum number of instances.
1521-1569: Ensure proper handling of multiple input SLP files.The function
test_sleap_track_mult_input_slpcorrectly handles multiple input SLP files by copying them into a temporary directory and running thesleap-trackcommand on each copy. The assertions ensure that the predictions files are correctly generated for each input file.
1571-1618: Ensure proper handling of multiple input SLP and MP4 files.The function
test_sleap_track_mult_input_slp_mp4correctly handles multiple input SLP and MP4 files by copying them into a temporary directory and running thesleap-trackcommand on each copy. The assertions ensure that the predictions files are correctly generated for each input file.
1620-1664: Ensure proper handling of multiple input MP4 files.The function
test_sleap_track_mult_input_mp4correctly handles multiple input MP4 files by copying them into a temporary directory and running thesleap-trackcommand on each copy. The assertions ensure that the predictions files are correctly generated for each input file.
1666-1715: Ensure proper handling of multiple input files with specified output directory.The function
test_sleap_track_output_multcorrectly handles multiple input MP4 files by copying them into a temporary directory and running thesleap-trackcommand on each copy with a specified output directory. The assertions ensure that the predictions files are correctly generated in the specified output directory.
1717-1750: Ensure proper handling of invalid output path.The function
test_sleap_track_invalid_outputcorrectly handles an invalid output path by raising aValueErrorexception. The test ensures that thesleap-trackcommand correctly handles invalid output paths.Tools
Ruff
1726-1726: Undefined name
as_posix(F821)
1752-1769: Ensure proper handling of invalid input path.The function
test_sleap_track_invalid_inputcorrectly handles an invalid input path by raising aValueErrorexception. The test ensures that thesleap-trackcommand correctly handles invalid input paths.
1521-1569: Ensure proper handling of multiple input SLP files with different tracking options.The function
test_sleap_track_mult_input_slpcorrectly handles multiple input SLP files by copying them into a temporary directory and running thesleap-trackcommand on each copy with different tracking options. The assertions ensure that the predictions files are correctly generated for each input file and tracking option.
1571-1618: Ensure proper handling of multiple input SLP and MP4 files with different tracking options.The function
test_sleap_track_mult_input_slp_mp4correctly handles multiple input SLP and MP4 files by copying them into a temporary directory and running thesleap-trackcommand on each copy with different tracking options. The assertions ensure that the predictions files are correctly generated for each input file and tracking option.
1621-1664: Ensure proper handling of multiple input MP4 files with different tracking options.The function
test_sleap_track_mult_input_mp4correctly handles multiple input MP4 files by copying them into a temporary directory and running thesleap-trackcommand on each copy with different tracking options. The assertions ensure that the predictions files are correctly generated for each input file and tracking option.
tests/nn/test_inference.py
Outdated
| ): | ||
|
|
||
| output_path = Path(tmpdir, "output_file.slp").as_posix() | ||
| output_path = Path(tmpdir, "output_file.slp"),as_posix() |
There was a problem hiding this comment.
Fix the typo in defining the output_path.
The output_path is defined incorrectly due to a typo. The correct method should be used to define the path.
- output_path = Path(tmpdir, "output_file.slp"),as_posix()
+ output_path = Path(tmpdir, "output_file.slp").as_posix()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| output_path = Path(tmpdir, "output_file.slp"),as_posix() | |
| output_path = Path(tmpdir, "output_file.slp").as_posix() |
Tools
Ruff
1726-1726: Undefined name
as_posix(F821)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/nn/test_inference.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/nn/test_inference.py
On further assessment during subgroup: let's actually just not do multiple SLP file support. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (4 hunks)
- tests/nn/test_inference.py (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- sleap/nn/inference.py
- tests/nn/test_inference.py
| ) | ||
| output_path_obj = Path(output_path) | ||
| # Create the containing directory if needed. | ||
| output_path_obj.parent.mkdir(exist_ok=True, parents=True) |
There was a problem hiding this comment.
is this what you wanted to do, or did you want to make the directory itself instead of the parent?
* Remove no-op code from #1498 * Add options to set background color when exporting video (#1328) * implement #921 * simplified form / refractor * Add test function and update cli docs * Improve test function to check background color * Improve comments * Change background options to lowercase * Use coderabbitai suggested `fill` --------- Co-authored-by: Shrivaths Shyam <[email protected]> Co-authored-by: Liezl Maree <[email protected]> * Increase range on batch size (#1513) * Increase range on batch size * Set maximum to a factor of 2 * Set default callable for `match_lists_function` (#1520) * Set default for `match_lists_function` * Move test code to official tests * Check using expected values * Allow passing in `Labels` to `app.main` (#1524) * Allow passing in `Labels` to `app.main` * Load the labels object through command * Add warning when unable to switch back to CPU mode * Replace (broken) `--unrag` with `--ragged` (#1539) * Fix unrag always set to true in sleap-export * Replace unrag with ragged * Fix typos * Add function to create app (#1546) * Refactor `AddInstance` command (#1561) * Refactor AddInstance command * Add staticmethod wrappers * Return early from set_visible_nodes * Import DLC with uniquebodyparts, add Tracks (#1562) * Import DLC with uniquebodyparts, add Tracks * add tests * correct tests * Make the hdf5 videos store as int8 format (#1559) * make the hdf5 video dataset type as proper int8 by padding with zeros * add gzip compression * Scale new instances to new frame size (#1568) * Fix typehinting in `AddInstance` * brought over changes from my own branch * added suggestions * Ensured google style comments --------- Co-authored-by: roomrys <[email protected]> Co-authored-by: sidharth srinath <[email protected]> * Fix package export (#1619) Add check for empty videos * Add resize/scroll to training GUI (#1565) * Make resizable training GUI and add adaptive scroll bar * Set a maximum window size --------- Co-authored-by: Liezl Maree <[email protected]> * support loading slp files with non-compound types and str in metadata (#1566) Co-authored-by: Liezl Maree <[email protected]> * change inference pipeline option to tracking-only (#1666) change inference pipeline none option to tracking-only * Add ABL:AOC 2023 Workshop link (#1673) * Add ABL:AOC 2023 Workshop link * Trigger website build * Graceful failing with seeking errors (#1712) * Don't try to seek to faulty last frame on provider initialization * Catch seeking errors and pass * Lint * Fix IndexError for hdf5 file import for single instance analysis files (#1695) * Fix hdf5 read for single instance analysis files * Add test * Small test files * removing unneccessary fixtures * Replace imgaug with albumentations (#1623) What's the worst that could happen? * Initial commit * Fix augmentation * Update more deps requirements * Use pip for installing albumentations and avoid reinstalling OpenCV * Update other conda envs * Fix out of bounds albumentations issues and update dependencies (#1724) * Install albumentations using conda-forge in environment file * Conda install albumentations * Add ndx-pose to pypi requirements * Keep out of bounds points * Black * Add ndx-pose to conda install in environment file * Match environment file without cuda * Ordered dependencies * Add test * Delete comments * Add conda packages to mac environment file * Order dependencies in pypi requirements * Add tests with zeroes and NaNs for augmentation * Back * Black * Make comment one line * Add todo for later * Black * Update to new TensorFlow conda package (#1726) * Build conda package locally * Try 2.8.4 * Merge develop into branch to fix dependencies * Change tensorflow version to 2.7.4 in where conda packages are used * Make tensorflow requirements in pypi looser * Conda package has TensorFlow 2.7.0 and h5py and numpy installed via conda * Change tensorflow version in `environment_no_cuda.yml` to test using CI * Test new sleap/tensorflow package * Reset build number * Bump version * Update mac deps * Update to Arm64 Mac runners * pin `importlib-metadata` * Pin more stuff on mac * constrain `opencv` version due to new qt dependencies * Update more mac stuff * Patches to get to green * More mac skipping --------- Co-authored-by: Talmo Pereira <[email protected]> Co-authored-by: Talmo Pereira <[email protected]> * Fix CI on macosx-arm64 (#1734) * Build conda package locally * Try 2.8.4 * Merge develop into branch to fix dependencies * Change tensorflow version to 2.7.4 in where conda packages are used * Make tensorflow requirements in pypi looser * Conda package has TensorFlow 2.7.0 and h5py and numpy installed via conda * Change tensorflow version in `environment_no_cuda.yml` to test using CI * Test new sleap/tensorflow package * Reset build number * Bump version * Update mac deps * Update to Arm64 Mac runners * pin `importlib-metadata` * Pin more stuff on mac * constrain `opencv` version due to new qt dependencies * Update more mac stuff * Patches to get to green * More mac skipping * Re-enable mac tests * Handle GPU re-init * Fix mac build CI * Widen tolerance for movenet correctness test * Fix build ci * Try for manual build without upload * Try to reduce training CI time * Rework actions * Fix miniforge usage * Tweaks * Fix build ci * Disable manual build * Try merging CI coverage * GPU/CPU usage in tests * Lint * Clean up * Fix test skip condition * Remove scratch test --------- Co-authored-by: eberrigan <[email protected]> * Add option to export to CSV via sleap-convert and API (#1730) * Add csv as a format option * Add analysis to format * Add csv suffix to output path * Add condition for csv analysis file * Add export function to Labels class * delete print statement * lint * Add `analysis.csv` as parametrize input for `sleap-convert` tests * test `export_csv` method added to `Labels` class * black formatting * use `Path` to construct filename * add `analysis.csv` to cli guide for `sleap-convert` --------- Co-authored-by: Talmo Pereira <[email protected]> * Only propagate Transpose Tracks when propagate is checked (#1748) Fix always-propagate transpose tracks issue * View Hyperparameter nonetype fix (#1766) Pass config getter argument to fetch hyperparameters * Adding ragged metadata to `info.json` (#1765) Add ragged metadata to info.json file * Add batch size to GUI for inference (#1771) * Fix conda builds (#1776) * test conda packages in a test environment as part of CI * do not test sleap import using conda build * use github environment variables to define build path for each OS in the matrix and add print statements for testing * figure out paths one OS at a time * github environment variables work in subsequent steps not current step * use local builds first * print env info * try simple environment creation * try conda instead of mamba * fix windows build path * fix windows build path * add comment to reference pull request * remove test stage from conda build for macs and test instead by creating the environment in a workflow * test workflow by pushing to current branch * test conda package on macos runner * Mac build does not need nvidia channel * qudida and albumentations are conda installed now * add comment with original issue * use python 3.9 * use conda match specifications syntax * make print statements more readable for troubleshooting python versioning * clean up build file * update version for pre-release * add TODO * add tests for conda packages before uploading * update ci comments and branches * remove macos test of pip wheel since python 3.9 is not supported by setup-python action * Upgrade build actions for release (#1779) * update `build.yml` so it matches updates from `build_manual.yml` * test `build.yml` without uploading * test again using build_manual.yml * build pip wheel with Ubuntu and turn off caching so build.yml exactly matches build_manual.yml * `build.yml` on release only and upload * testing caching * `use-only-tar-bz2: true` makes environment unsolvable, change it back * Update .github/workflows/build_manual.yml Co-authored-by: Liezl Maree <[email protected]> * Update .github/workflows/build.yml Co-authored-by: Liezl Maree <[email protected]> * bump pre-release version * fix version for pre-release * run build and upload on release! * try setting `CACHE_NUMBER` to 1 with `use-only-tar-bz2` set to true * increasing the cache number to reset the cache does work when `use-only-tar-bz2` is set to true * publish and upload on release only --------- Co-authored-by: Liezl Maree <[email protected]> * Add ZMQ support via GUI and CLI (#1780) * Add ZMQ support via GUI and CLI, automatic port handler, separate utils module for the functions * Change menu name to match deleting predictions beyond max instance (#1790) Change menu and function names * Fix website build and remove build cache across workflows (#1786) * test with build_manual on push * comment out caching in build manual * remove cache step from builad manual since environment resolves when this is commented out * comment out cache in build ci * remove cache from build on release * remove cache from website build * test website build on push * add name to checkout step * update checkout to v4 * update checkout to v4 in build ci * remove cache since build ci works without it * update upload-artifact to v4 in build ci * update second chechout to v4 in build ci * update setup-python to v5 in build ci * update download-artifact to v4 in build ci * update checkout to v4 in build ci * update checkout to v4 in website build * update setup-miniconda to v3.0.3 in website build * update actions-gh-pages to v4 in website build * update actions checkout and setup-python in ci * update checkout action in ci to v4 * pip install lxml[html_clean] because of error message during action * add error message to website to explain why pip install lxml[html_clean] * remove my branch for pull request * Bump to 1.4.1a1 (#1791) * bump versions to 1.4.1a1 * we can change the version on the installation page since this will be merged into the develop branch and not main * Fix windows conda package upload and build ci (#1792) * windows OS is 2022 not 2019 on runner * upload windows conda build manually but not pypi build * remove comment and run build ci * change build manual back so that it doesn't upload * remove branch from build manual * update installation docs for 1.4.1a1 * Fix zmq inference (#1800) * Ensure that we always pass in the zmq_port dict to LossViewer * Ensure zmq_ports has correct keys inside LossViewer * Use specified controller and publish ports for first attempted addresses * Add test for ports being set in LossViewer * Add max attempts to find unused port * Fix find free port loop and add for controller port also * Improve code readablility and reuse * Improve error message when unable to find free port * Set selected instance to None after removal (#1808) * Add test that selected instance set to None after removal * Set selected instance to None after removal * Add `InstancesList` class to handle backref to `LabeledFrame` (#1807) * Add InstancesList class to handle backref to LabeledFrame * Register structure/unstructure hooks for InstancesList * Add tests for the InstanceList class * Handle case where instance are passed in but labeled_frame is None * Add tests relevant methods in LabeledFrame * Delegate setting frame to InstancesList * Add test for PredictedInstance.frame after complex merge * Add todo comment to not use Instance.frame * Add rtest for InstasnceList.remove * Use normal list for informative `merged_instances` * Add test for copy and clear * Add copy and clear methods, use normal lists in merge method * Bump to v1.4.1a2 (#1835) bump to 1.4.1a2 * Updated trail length viewing options (#1822) * updated trail length optptions * Updated trail length options in the view menu * Updated `prefs` to include length info from `preferences.yaml` * Added trail length as method of `MainWindow` * Updated trail length documentation * black formatting --------- Co-authored-by: Keya Loding <[email protected]> * Handle case when no frame selection for trail overlay (#1832) * Menu option to open preferences directory and update to util functions to pathlib (#1843) * Add menu to view preferences directory and update to pathlib * text formatting * Add `Keep visualizations` checkbox to training GUI (#1824) * Renamed save_visualizations to view_visualizations for clarity * Added Delete Visualizations button to the training pipeline gui, exposed del_viz_predictions config option to the user * Reverted view_ back to save_ and changed new training checkbox to Keep visualization images after training. * Fixed keep_viz config option state override bug and updated keep_viz doc description * Added test case for reading training CLI argument correctly * Removed unnecessary testing code * Creating test case to check for viz folder * Finished tests to check CLI argument reading and viz directory existence * Use empty string instead of None in cli args test * Use keep_viz_images false in most all test configs (except test to override config) --------- Co-authored-by: roomrys <[email protected]> * Allowing inference on multiple videos via `sleap-track` (#1784) * implementing proposed code changes from issue #1777 * comments * configuring output_path to support multiple video inputs * fixing errors from preexisting test cases * Test case / code fixes * extending test cases for mp4 folders * test case for output directory * black and code rabbit fixes * code rabbit fixes * as_posix errors resolved * syntax error * adding test data * black * output error resolved * edited for push to dev branch * black * errors fixed, test cases implemented * invalid output test and invalid input test * deleting debugging statements * deleting print statements * black * deleting unnecessary test case * implemented tmpdir * deleting extraneous file * fixing broken test case * fixing test_sleap_track_invalid_output * removing support for multiple slp files * implementing talmo's comments * adding comments * Add object keypoint similarity method (#1003) * Add object keypoint similarity method * fix max_tracking * correct off-by-one error * correct off-by-one error * Generate suggestions using max point displacement threshold (#1862) * create function max_point_displacement, _max_point_displacement_video. Add to yaml file. Create test for new function . . . will need to edit * remove unnecessary for loop, calculate proper displacement, adjusted tests accordingly * Increase range for displacement threshold * Fix frames not found bug * Return the latter frame index * Lint --------- Co-authored-by: roomrys <[email protected]> * Added Three Different Cases for Adding a New Instance (#1859) * implemented paste with offset * right click and then default will paste the new instance at the location of the cursor * modified the logics for creating new instance * refined the logic * fixed the logic for right click * refined logics for adding new instance at a specific location * Remove print statements * Comment code * Ensure that we choose a non nan reference node * Move OOB nodes to closest in-bounds position --------- Co-authored-by: roomrys <[email protected]> * Allow csv and text file support on sleap track (#1875) * initial changes * csv support and test case * increased code coverage * Error fixing, black, deletion of (self-written) unused code * final edits * black * documentation changes * documentation changes * Fix GUI crash on scroll (#1883) * Only pass wheelEvent to children that can handle it * Add test for wheelEvent * Fix typo to allow rendering videos with mp4 (Mac) (#1892) Fix typo to allow rendering videos with mp4 * Do not apply offset when double clicking a `PredictedInstance` (#1888) * Add offset argument to newInstance and AddInstance * Apply offset of 10 for Add Instance menu button (Ctrl + I) * Add offset for docks Add Instance button * Make the QtVideoPlayer context menu unit-testable * Add test for creating a new instance * Add test for "New Instance" button in `InstancesDock` * Fix typo in docstring * Add docstrings and typehinting * Remove unused imports and sort imports * Refactor video writer to use imageio instead of skvideo (#1900) * modify `VideoWriter` to use imageio with ffmpeg backend * check to see if ffmpeg is present * use the new check for ffmpeg * import imageio.v2 * add imageio-ffmpeg to environments to test * using avi format for now * remove SKvideo videowriter * test `VideoWriterImageio` minimally * add more documentation for ffmpeg * default mp4 for ffmpeg should be mp4 * print using `IMAGEIO` when using ffmpeg * mp4 for ffmpeg * use mp4 ending in test * test `VideoWriterImageio` with avi file extension * test video with odd size * remove redundant filter since imageio-ffmpeg resizes automatically * black * remove unused import * use logging instead of print statement * import cv2 is needed for resize * remove logging * Use `Video.from_filename` when structuring videos (#1905) * Use Video.from_filename when structuring videos * Modify removal_test_labels to have extension in filename * Use | instead of + in key commands (#1907) * Use | instead of + in key commands * Lint * Replace QtDesktop widget in preparation for PySide6 (#1908) * Replace to-be-depreciated QDesktopWidget * Remove unused imports and sort remaining imports * Remove unsupported |= operand to prepare for PySide6 (#1910) Fixes TypeError: unsupported operand type(s) for |=: 'int' and 'Option' * Use positional argument for exception type (#1912) traceback.format_exception has changed it's first positional argument's name from etype to exc in python 3.7 to 3.10 * Replace all Video structuring with Video.cattr() (#1911) * Remove unused AsyncVideo class (#1917) Remove unused AsyncVideo * Refactor `LossViewer` to use matplotlib (#1899) * use updated syntax for QtAgg backend of matplotlib * start add features to `MplCanvas` to replace QtCharts features in `LossViewer` (untested) * remove QtCharts imports and replace with MplCanvas * remove QtCharts imports and replace with MplCanvas * start using MplCanvas in LossViwer instead of QtCharts (untested) * use updated syntax * Uncomment all commented out QtChart * Add debug code * Refactor monitor to use LossViewer._init_series method * Add monitor only debug code * Add methods for setting up axes and legend * Add the matplotlib canvas to the widget * Resize axis with data (no log support yet) * Try using PathCollection for "batch" * Get "batch" plotting with ax.scatter (no log support yet) * Add log support * Add a _resize_axis method * Modify init_series to work for ax.plot as well * Use matplotlib to plot epoch_loss line * Add method _add_data_to_scatter * Add _add_data_to_plot method * Add docstring to _resize_axes * Add matplotlib plot for val_loss * Add matplotlib scatter for val_loss_best * Avoid errors with setting log scale before any positive values * Add x and y axes labels * Set title (removing html tags) * Add legend * Adjust positioning of plot * Lint * Leave MplCanvas unchanged * Removed unused training_monitor.LossViewer * Resize fonts * Move legend outside of plot * Add debug code for montitor aesthetics * Use latex formatting to bold parts of title * Make axes aesthetic * Add midpoint grid lines * Set initial limits on x and y axes to be 0+ * Ensure x axis minimum is always resized to 0+ * Adjust plot to account for plateau patience title * Add debug code for plateau patience title line * Lint * Set thicker line width * Remove unused import * Set log axis on initialization * Make tick labels smaller * Move plot down a smidge * Move ylabel left a bit * Lint * Add class LossPlot * Refactor LossViewer to use LossPlot * Remove QtCharts code * Remove debug codes * Allocate space for figure items based on item's size * Refactor LossPlot to use underscores for internal methods * Ensure y_min, y_max not equal Otherwise we get an unnecessary teminal message: UserWarning: Attempting to set identical bottom == top == 3.0 results in singular transformations; automatically expanding. self.axes.set_ylim(y_min, y_max) --------- Co-authored-by: roomrys <[email protected]> Co-authored-by: roomrys <[email protected]> * Refactor `LossViewer` to use underscores for internal method names (#1919) Refactor LossViewer to use underscores for internal method names * Manually handle `Instance.from_predicted` structuring when not `None` (#1930) * Use `tf.math.mod` instead of `%` (#1931) * Option for Max Stride to be 128 (#1941) Co-authored-by: Max Weinberg <[email protected]> * Add discussion comment workflow (#1945) * Add a bot to autocomment on workflow * Use github markdown warning syntax * Add a multiline warning * Change happy coding to happy SLEAPing Co-authored-by: Talmo Pereira <[email protected]> --------- Co-authored-by: roomrys <[email protected]> Co-authored-by: Talmo Pereira <[email protected]> * Add comment on issue workflow (#1946) * Add workflow to test conda packages (#1935) * Add missing imageio-ffmpeg to meta.ymls (#1943) * Update installation docs 1.4.1 (#1810) * [wip] Updated installation docs * Add tabs for different OS installations * Move installation methods to tabs * Use tabs.css * FIx styling error (line under last tab in terminal hint) * Add installation instructions before TOC * Replace mamba with conda * Lint * Find good light colors not switching when change dark/light themes * Get color scheme switching with dark/light toggle button * Upgrade website build dependencies * Remove seemingly unneeded dependencies from workflow * Add myst-nb>=0.16.0 lower bound * Trigger dev website build * Fix minor typo in css * Add miniforge and one-liner installs for package managers --------- Co-authored-by: roomrys <[email protected]> Co-authored-by: Talmo Pereira <[email protected]> * Add imageio dependencies for pypi wheel (#1950) Add imagio dependencies for pypi wheel Co-authored-by: roomrys <[email protected]> * Do not always color skeletons table black (#1952) Co-authored-by: roomrys <[email protected]> * Remove no module named work error (#1956) * Do not always color skeletons table black * Remove offending (possibly unneeded) line that causes the no module named work error to print in terminal * Remove offending (possibly unneeded) line that causes the no module named work error to print in terminal * Remove accidentally added changes * Add (failing) test to ensure menu-item updates with state change * Reconnect callback for menu-item (using lambda) * Add (failing) test to ensure menu-item updates with state change Do not assume inital state * Reconnect callback for menu-item (using lambda) --------- Co-authored-by: roomrys <[email protected]> * Add `normalized_instance_similarity` method (#1939) * Add normalize function * Expose normalization function * Fix tests * Expose object keypoint sim function * Fix tests * Handle skeleton decoding internally (#1961) * Reorganize (and add) imports * Add (and reorganize) imports * Modify decode_preview_image to return bytes if specified * Implement (minimally tested) replace_jsonpickle_decode * Add support for using idx_to_node map i.e. loading from Labels (slp file) * Ignore None items in reduce_list * Convert large function to SkeletonDecoder class * Update SkeletonDecoder.decode docstring * Move decode_preview_image to SkeletonDecoder * Use SkeletonDecoder instead of jsonpickle in tests * Remove unused imports * Add test for decoding dict vs tuple pystates * Handle skeleton encoding internally (#1970) * start class `SkeletonEncoder` * _encoded_objects need to be a dict to add to * add notebook for testing * format * fix type in docstring * finish classmethod for encoding Skeleton as a json string * test encoded Skeleton as json string by decoding it * add test for decoded encoded skeleton * update jupyter notebook for easy testing * constraining attrs in dev environment to make sure decode format is always the same locally * encode links first then encode source then target then type * save first enconding statically as an input to _get_or_assign_id so that we do not always get py/id * save first encoding statically * first encoding is passed to _get_or_assign_id * use first_encoding variable to determine if we should assign a py/id * add print statements for debugging * update notebook for easy testing * black * remove comment * adding attrs constraint to show this passes for certain attrs version only * add import * switch out jsonpickle.encode * oops remove import * can attrs be unconstrained? * forgot comma * pin attrs for testing * test Skeleton from json, template, with symmetries, and template * use SkeletonEncoder.encode * black * try removing None values in EdgeType reduced * Handle case when nodes are replaced by integer indices from caller * Remove prototyping notebook * Remove attrs pins * Remove sort keys (which flips the neccessary ordering of our py/ids) * Do not add extra indents to encoded file * Only append links after fully encoded (fat-finger) * Remove outdated comment * Lint --------- Co-authored-by: Talmo Pereira <[email protected]> Co-authored-by: roomrys <[email protected]> * Pin ndx-pose<0.2.0 (#1978) * Pin ndx-pose<0.2.0 * Typo * Sort encoded `Skeleton` dictionary for backwards compatibility (#1975) * Add failing test to check that encoded Skeleton is sorted * Sort Skeleton dictionary before encoding * Remove unused import * Disable comment bot for now * Fix COCO Dataset Loading for Invisible Keypoints (#2035) Update coco.py # Fix COCO Dataset Loading for Invisible Keypoints ## Issue When loading COCO datasets, keypoints marked as invisible (flag=0) are currently skipped and later placed randomly within the instance's bounding box. However, in COCO format, these keypoints may still have valid coordinate information that should be preserved (see toy_dataset for expected vs. current behavior). ## Changes Modified the COCO dataset loading logic to: - Check if invisible keypoints (flag=0) have non-zero coordinates - If coordinates are (0,0), skip the point (existing behavior) - If coordinates are not (0,0), create the point at those coordinates but mark it as not visible - Maintain existing behavior for visible (flag=2) and labeled * Lint * Add tracking score as seekbar header options (#2047) * Add `tracking_score` as a constructor arg for `PredictedInstance` * Add `tracking_score` to ID models * Add fixture with tracking scores * Add tracking score to seekbar header * Add bonsai guide for sleap docs (#2050) * [WIP] Add bonsai guide page * Add more information to the guide with images * add branch for website build * Typos * fix links * Include suggestions * Add more screenshots and refine the doc * Remove branch from website workflow * Completed documentation edits from PR made by reviewer + review bot. --------- Co-authored-by: Shrivaths Shyam <[email protected]> Co-authored-by: Liezl Maree <[email protected]> * Don't mark complete on instance scaling (#2049) * Add check for instances with track assigned before training ID models (#2053) * Add menu item for deleting instances beyond frame limit (#1797) * Add menu item for deleting instances beyond frame limit * Add test function to test the instances returned * typos * Update docstring * Add frame range form * Extend command to use frame range --------- Co-authored-by: Talmo Pereira <[email protected]> * Highlight instance box on hover (#2055) * Make node marker and label sizes configurable via preferences (#2057) * Make node marker and label sizes configurable via preferences * Fix test * Enable touchpad pinch to zoom (#2058) * Fix import PySide2 -> qtpy (#2065) * Fix import PySide2 -> qtpy * Remove unnecessary print statements. * Add channels for pip conda env (#2067) * Add channels for pypi conda env * Trigger dev website build * Separate the video name and its filepath columns in `VideoTablesModel` (#2052) * add option to show video names with filepath * add doc * new feature added successfully * delete unnecessary code * remove attributes from video object * Update dataviews.py * remove all properties * delete toggle option * remove video show * fix the order of the columns * remove options * Update sleap/gui/dataviews.py Co-authored-by: Liezl Maree <[email protected]> * Update sleap/gui/dataviews.py Co-authored-by: Liezl Maree <[email protected]> * use pathlib instead of substrings * Update dataviews.py Co-authored-by: Liezl Maree <[email protected]> * Use Path instead of pathlib.Path and sort imports and remove unused imports * Use item.filename instead of getattr --------- Co-authored-by: Liezl Maree <[email protected]> * Make status bar dependent on UI mode (#2063) * remove bug for dark mode * fix toggle case --------- Co-authored-by: Liezl Maree <[email protected]> * Bump version to 1.4.1 (#2062) * Bump version to 1.4.1 * Trigger conda/pypi builds (no upload) * Trigger website build * Add dev channel to installation instructions --------- Co-authored-by: Talmo Pereira <[email protected]> * Add -c sleap/label/dev channel for win/linux - also trigger website build --------- Co-authored-by: Scott Yang <[email protected]> Co-authored-by: Shrivaths Shyam <[email protected]> Co-authored-by: getzze <[email protected]> Co-authored-by: Lili Karashchuk <[email protected]> Co-authored-by: Sidharth Srinath <[email protected]> Co-authored-by: sidharth srinath <[email protected]> Co-authored-by: Talmo Pereira <[email protected]> Co-authored-by: KevinZ0217 <[email protected]> Co-authored-by: Elizabeth <[email protected]> Co-authored-by: Talmo Pereira <[email protected]> Co-authored-by: eberrigan <[email protected]> Co-authored-by: vaibhavtrip29 <[email protected]> Co-authored-by: Keya Loding <[email protected]> Co-authored-by: Keya Loding <[email protected]> Co-authored-by: Hajin Park <[email protected]> Co-authored-by: Elise Davis <[email protected]> Co-authored-by: gqcpm <[email protected]> Co-authored-by: Andrew Park <[email protected]> Co-authored-by: roomrys <[email protected]> Co-authored-by: MweinbergUmass <[email protected]> Co-authored-by: Max Weinberg <[email protected]> Co-authored-by: DivyaSesh <[email protected]> Co-authored-by: Felipe Parodi <[email protected]> Co-authored-by: croblesMed <[email protected]>
Description
Adding an option to add a directory to
sleap-trackto allow the command to iterate through the files in the directory and run inferences on all of them.Types of changes
Does this address any currently open issues?
#1777
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests