Allow csv and text file support on sleap track #1875
Conversation
WalkthroughThe recent updates to the Changes
Possibly related issues
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: 7
Outside diff range, codebase verification and nitpick comments (2)
sleap/nn/inference.py (2)
5288-5289: Update function docstring to reflect new return type.The function docstring should be updated to reflect the new return type, which now includes lists of providers, data paths, and output paths.
- `(provider, data_path)` with the data `Provider` and path to the data that was specified in the args. + `(provider_list, data_path_list, output_path_list)` with the data `Provider`, path to the data that was specified in the args, and list of output paths if a CSV file was inputted.
5302-5302: Remove unnecessary blank line.Remove the unnecessary blank line for better readability.
-
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
5313-5313: Undefined name
pandas(F821)
5325-5325: Undefined name
pandas(F821)
5327-5327: Undefined name
pandas(F821)
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (7)
tests/nn/test_inference.py (4)
1751-1756: Function signature and setup.The function signature and setup are correct, but consider adding a docstring to describe the purpose and steps of the test.
def test_sleap_track_csv_input( min_centroid_model_path: str, min_centered_instance_model_path: str, centered_pair_vid_path, tmpdir, ): """ Test sleap_track with CSV input. Args: min_centroid_model_path: Path to the centroid model. min_centered_instance_model_path: Path to the centered instance model. centered_pair_vid_path: Path to the video file. tmpdir: Temporary directory for test files. """
1758-1769: Setup temporary directory and copy video files.The setup correctly creates a temporary directory and copies the video files into it. However, consider using a more descriptive variable name for
num_copies.- num_copies = 3 + num_video_copies = 3
1775-1781: Create CSV file.The CSV file creation is correct, but consider adding a check to ensure the file is created successfully.
+ assert csv_file_path.exists(), "CSV file was not created successfully."
1794-1801: Run inference and assert output files.The assertions correctly check for the existence of the expected output files. However, consider adding more detailed assertions to verify the content of the output files.
+ for expected_output_file in output_paths: + assert Path(expected_output_file).exists(), f"Output file {expected_output_file} does not exist." + # Add more detailed assertions to verify the content of the output files if necessary.sleap/nn/inference.py (3)
5289-5290: Update the docstring to reflect the new return type.The docstring should mention that the function now returns lists of providers, data paths, and output paths.
- `(provider_list, data_path_list, output_path_list)` with the data `Provider`, path to the data - that was specified in the args, and list out output paths if a csv file was inputed. + `(provider_list, data_path_list, output_path_list)` with the data `Provider`, list of data paths, + and list of output paths if a CSV file was inputted.
5360-5360: Add a comment to explain the purpose of the loop.Adding a comment will help future developers understand the purpose of this loop.
+ # Iterate over each file path to create providers. for file_path in raw_data_path_list:
5533-5534: Update the return statement to reflect the new return type.Ensure the return statement matches the updated return type in the docstring.
- provider_list, data_path_list, output_path_list = _make_provider_from_cli(args) + return provider_list, data_path_list, output_path_list
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (6 hunks)
- tests/nn/test_inference.py (2 hunks)
Additional comments not posted (4)
tests/nn/test_inference.py (2)
1770-1773: Generate output paths.The generation of output paths is correct and follows the expected pattern.
1785-1791: Create sleap-track command.The command is correctly constructed. Ensure that the command arguments are valid and that the
sleap_trackfunction handles the CSV input as expected.sleap/nn/inference.py (2)
5304-5305: Initializeoutput_path_listas an empty list.To maintain consistency and avoid potential issues, initialize
output_path_listas an empty list instead ofNone.- output_path_list = None + output_path_list = []
5356-5357: Initializeprovider_listanddata_path_listas empty lists.To maintain consistency, initialize
provider_listanddata_path_listas empty lists.- provider_list = [] - data_path_list = [] + provider_list: List[Provider] = [] + data_path_list: List[str] = []Likely invalid or redundant comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1875 +/- ##
===========================================
+ Coverage 73.30% 74.36% +1.05%
===========================================
Files 134 135 +1
Lines 24087 24781 +694
===========================================
+ Hits 17658 18429 +771
+ Misses 6429 6352 -77 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
tests/nn/test_inference.py (2)
1771-1821: Consider adding assertions to validate CSV file parsing.While the test ensures that the expected output files are created, it would be beneficial to add assertions that check if the CSV file is correctly read and parsed.
+ # Assert CSV file is correctly read and parsed + with open(csv_file_path, mode="r") as csv_file: + csv_reader = csv.reader(csv_file) + headers = next(csv_reader) + assert headers == ["data_path", "output_path"] + rows = list(csv_reader) + assert len(rows) == num_copies
1866-1909: Consider adding assertions to validate text file parsing.While the test ensures that the expected output files are created, it would be beneficial to add assertions that check if the text file is correctly read and parsed.
+ # Assert text file is correctly read and parsed + with open(txt_file_path, mode="r") as txt_file: + lines = txt_file.readlines() + assert len(lines) == num_copies + for i, line in enumerate(lines): + assert line.strip() == str(file_paths[i])sleap/nn/inference.py (1)
5289-5290: Clarify the return type in the docstring.The docstring should clearly specify that the function returns a tuple of lists:
(provider_list, data_path_list, output_path_list).- `(provider_list, data_path_list, output_path_list)` with the data `Provider`, path to the data - that was specified in the args, and list out output paths if a csv file was inputed. + `(provider_list, data_path_list, output_path_list)` with the data `Provider`, list of data paths, + and list of output paths if a CSV file was provided.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (6 hunks)
- tests/nn/test_inference.py (3 hunks)
Additional comments not posted (5)
tests/nn/test_inference.py (1)
1824-1863: LGTM!The test function is well-structured and covers important edge cases for invalid CSV inputs.
sleap/nn/inference.py (4)
5304-5305: Initializeoutput_path_listas an empty list.To maintain consistency and avoid potential issues, initialize
output_path_listas an empty list instead ofNone.- output_path_list = None + output_path_list = []
5312-5329: Initializeoutput_path_listas an empty list if the column exists.To ensure consistency, initialize
output_path_listas an empty list if the 'output_path' column exists.- output_path_list = df["output_path"].tolist() + output_path_list = df["output_path"].tolist() if "output_path" in df.columns else []
5350-5353: Filter video files more accurately.Ensure that only supported video files are included in
raw_data_path_list.- raw_data_path_list = [ - file_path for file_path in data_path_obj.iterdir() if file_path.is_file() - ] + supported_extensions = {".mp4", ".avi", ".mov", ".mkv"} + raw_data_path_list = [ + file_path for file_path in data_path_obj.iterdir() + if file_path.is_file() and file_path.suffix.lower() in supported_extensions + ]
5388-5396: Improve error handling for video reading.Include the file name in the error message for better debugging.
- except Exception: - print(f"Error reading file: {file_path.as_posix()}") + except Exception as e: + print(f"Error reading file: {file_path.as_posix()}. Error: {e}")
sleap/nn/inference.py
Outdated
| if "data_path" in df.columns: | ||
| raw_data_path_list = df["data_path"].tolist() | ||
| else: | ||
| raise ValueError( | ||
| f"Column 'data_path' does not exist in the CSV file: {data_path}" | ||
| ) |
There was a problem hiding this comment.
Design choice: we may want to be less sensitive to column naming. It's probably fair to assume that the first column of the CSV is the input path and the second column is the output path.
It'd be great to add some logic to auto-detect if the column names are present in the first row or not, and ignore it appropriately. An easy way would be to just do a Path(df.iloc[0, 0]).exists() and if not (or if it's not a path -- not sure if pathlib does any checking), then assume that it's a column name row and skip it.
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (4)
tests/nn/test_inference.py (3)
1764-1803: Ensure temporary directory cleanup.Currently, the function does not ensure that the temporary directory is cleaned up after the test. Consider using
tmpdir's context manager to handle this.- slp_path = Path(tmpdir.mkdir("mp4_directory")) + with tmpdir.mkdir("mp4_directory") as slp_path:
1817-1854: Ensure temporary directory cleanup.Currently, the function does not ensure that the temporary directory is cleaned up after the test. Consider using
tmpdir's context manager to handle this.- csv_missing_column_path = tmpdir / "missing_column.csv" + with tmpdir as csv_missing_column_path:
1857-1890: Ensure temporary directory cleanup.Currently, the function does not ensure that the temporary directory is cleaned up after the test. Consider using
tmpdir's context manager to handle this.- slp_path = Path(tmpdir.mkdir("mp4_directory")) + with tmpdir.mkdir("mp4_directory") as slp_path:sleap/nn/inference.py (1)
5289-5290: Update the docstring to reflect new return values.The docstring should be updated to reflect that the function now returns a tuple with
provider_list,data_path_list, andoutput_path_list.- `(provider_list, data_path_list, output_path_list)` with the data `Provider`, path to the data - that was specified in the args, and list out output paths if a csv file was inputed. + `(provider_list, data_path_list, output_path_list)` with the data `Provider`, path to the data + that was specified in the args, and list of output paths if a CSV file was provided.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (8 hunks)
- tests/nn/test_inference.py (7 hunks)
Additional context used
Ruff
sleap/nn/inference.py
5632-5632: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
Additional comments not posted (2)
sleap/nn/inference.py (2)
5304-5305: LGTM!Initializing
output_path_listtoNoneis appropriate.
5345-5354: LGTM!The logic for handling text files is straightforward and correct.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (8 hunks)
- tests/nn/test_inference.py (8 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
13-13:
tensorflow_hubimported but unusedRemove unused import:
tensorflow_hub(F401)
Additional comments not posted (1)
tests/nn/test_inference.py (1)
1823-1861: Ensure specific exception message is checked.Currently, the test only checks for a
ValueErrorexception. Ensure that the specific exception message is checked to verify the correct error is raised.- with pytest.raises(ValueError): + with pytest.raises(ValueError, match="Expected error message"):Likely invalid or redundant comment.
| def test_sleap_track_csv_input( | ||
| min_centroid_model_path: str, | ||
| min_centered_instance_model_path: str, | ||
| centered_pair_vid_path, | ||
| tmpdir, | ||
| ): | ||
|
|
||
| # Create temporary directory with the structured video files | ||
| slp_path = Path(tmpdir.mkdir("mp4_directory")) | ||
|
|
||
| # Copy and paste the video into the temp dir multiple times | ||
| num_copies = 3 | ||
| file_paths = [] | ||
| for i in range(num_copies): | ||
| # Construct the destination path with a unique name | ||
| dest_path = slp_path / f"centered_pair_vid_copy_{i}.mp4" | ||
| shutil.copy(centered_pair_vid_path, dest_path) | ||
| file_paths.append(dest_path) | ||
|
|
||
| # Generate output paths for each data_path | ||
| output_paths = [ | ||
| file_path.with_suffix(".TESTpredictions.slp") for file_path in file_paths | ||
| ] | ||
|
|
||
| # Create a CSV file with the file paths | ||
| csv_file_path = slp_path / "file_paths.csv" | ||
| with open(csv_file_path, mode="w", newline="") as csv_file: | ||
| csv_writer = csv.writer(csv_file) | ||
| csv_writer.writerow(["data_path", "output_path"]) | ||
| for data_path, output_path in zip(file_paths, output_paths): | ||
| csv_writer.writerow([data_path, output_path]) | ||
|
|
||
| slp_path_obj = Path(slp_path) | ||
|
|
||
| # Create sleap-track command | ||
| args = ( | ||
| f"{csv_file_path} --model {min_centroid_model_path} " | ||
| f"--tracking.tracker simple " | ||
| 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 = available_video_exts() | ||
|
|
||
| for file_path in slp_path_list: | ||
| if file_path.suffix in expected_extensions: | ||
| expected_output_file = file_path.with_suffix(".TESTpredictions.slp") | ||
| assert Path(expected_output_file).exists() | ||
|
|
There was a problem hiding this comment.
Ensure all expected output files are checked.
The current implementation only checks for .TESTpredictions.slp files. Ensure that all expected output files are checked, regardless of the input file extension.
- for file_path in slp_path_list:
- if file_path.suffix in expected_extensions:
- expected_output_file = file_path.with_suffix(".TESTpredictions.slp")
- assert Path(expected_output_file).exists()
+ for file_path in file_paths:
+ expected_output_file = file_path.with_suffix(".TESTpredictions.slp")
+ assert Path(expected_output_file).exists()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_csv_input( | |
| min_centroid_model_path: str, | |
| min_centered_instance_model_path: str, | |
| centered_pair_vid_path, | |
| tmpdir, | |
| ): | |
| # Create temporary directory with the structured video files | |
| slp_path = Path(tmpdir.mkdir("mp4_directory")) | |
| # Copy and paste the video into the temp dir multiple times | |
| num_copies = 3 | |
| file_paths = [] | |
| for i in range(num_copies): | |
| # Construct the destination path with a unique name | |
| dest_path = slp_path / f"centered_pair_vid_copy_{i}.mp4" | |
| shutil.copy(centered_pair_vid_path, dest_path) | |
| file_paths.append(dest_path) | |
| # Generate output paths for each data_path | |
| output_paths = [ | |
| file_path.with_suffix(".TESTpredictions.slp") for file_path in file_paths | |
| ] | |
| # Create a CSV file with the file paths | |
| csv_file_path = slp_path / "file_paths.csv" | |
| with open(csv_file_path, mode="w", newline="") as csv_file: | |
| csv_writer = csv.writer(csv_file) | |
| csv_writer.writerow(["data_path", "output_path"]) | |
| for data_path, output_path in zip(file_paths, output_paths): | |
| csv_writer.writerow([data_path, output_path]) | |
| slp_path_obj = Path(slp_path) | |
| # Create sleap-track command | |
| args = ( | |
| f"{csv_file_path} --model {min_centroid_model_path} " | |
| f"--tracking.tracker simple " | |
| 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 = available_video_exts() | |
| for file_path in slp_path_list: | |
| if file_path.suffix in expected_extensions: | |
| expected_output_file = file_path.with_suffix(".TESTpredictions.slp") | |
| assert Path(expected_output_file).exists() | |
| for file_path in file_paths: | |
| expected_output_file = file_path.with_suffix(".TESTpredictions.slp") | |
| assert Path(expected_output_file).exists() |
| def test_sleap_track_text_file_input( | ||
| min_centroid_model_path: str, | ||
| min_centered_instance_model_path: str, | ||
| centered_pair_vid_path, | ||
| tmpdir, | ||
| ): | ||
|
|
||
| # Create temporary directory with the structured video files | ||
| slp_path = Path(tmpdir.mkdir("mp4_directory")) | ||
|
|
||
| # Copy and paste the video into the temp dir multiple times | ||
| num_copies = 3 | ||
| file_paths = [] | ||
| for i in range(num_copies): | ||
| # Construct the destination path with a unique name | ||
| dest_path = slp_path / f"centered_pair_vid_copy_{i}.mp4" | ||
| shutil.copy(centered_pair_vid_path, dest_path) | ||
| file_paths.append(dest_path) | ||
|
|
||
| # Create a text file with the file paths | ||
| txt_file_path = slp_path / "file_paths.txt" | ||
| with open(txt_file_path, mode="w") as txt_file: | ||
| for file_path in file_paths: | ||
| txt_file.write(f"{file_path}\n") | ||
|
|
||
| slp_path_obj = Path(slp_path) | ||
|
|
||
| # Create sleap-track command | ||
| args = ( | ||
| f"{txt_file_path} --model {min_centroid_model_path} " | ||
| f"--tracking.tracker simple " | ||
| 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 = available_video_exts() | ||
|
|
||
| for file_path in slp_path_list: | ||
| if file_path.suffix in expected_extensions: | ||
| expected_output_file = Path(file_path).with_suffix(".predictions.slp") | ||
| assert Path(expected_output_file).exists() | ||
|
|
There was a problem hiding this comment.
Ensure all expected output files are checked.
The current implementation only checks for .predictions.slp files. Ensure that all expected output files are checked, regardless of the input file extension.
- for file_path in slp_path_list:
- if file_path.suffix in expected_extensions:
- expected_output_file = Path(file_path).with_suffix(".predictions.slp")
- assert Path(expected_output_file).exists()
+ for file_path in file_paths:
+ expected_output_file = file_path.with_suffix(".predictions.slp")
+ assert Path(expected_output_file).exists()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_text_file_input( | |
| min_centroid_model_path: str, | |
| min_centered_instance_model_path: str, | |
| centered_pair_vid_path, | |
| tmpdir, | |
| ): | |
| # Create temporary directory with the structured video files | |
| slp_path = Path(tmpdir.mkdir("mp4_directory")) | |
| # Copy and paste the video into the temp dir multiple times | |
| num_copies = 3 | |
| file_paths = [] | |
| for i in range(num_copies): | |
| # Construct the destination path with a unique name | |
| dest_path = slp_path / f"centered_pair_vid_copy_{i}.mp4" | |
| shutil.copy(centered_pair_vid_path, dest_path) | |
| file_paths.append(dest_path) | |
| # Create a text file with the file paths | |
| txt_file_path = slp_path / "file_paths.txt" | |
| with open(txt_file_path, mode="w") as txt_file: | |
| for file_path in file_paths: | |
| txt_file.write(f"{file_path}\n") | |
| slp_path_obj = Path(slp_path) | |
| # Create sleap-track command | |
| args = ( | |
| f"{txt_file_path} --model {min_centroid_model_path} " | |
| f"--tracking.tracker simple " | |
| 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 = available_video_exts() | |
| for file_path in slp_path_list: | |
| if file_path.suffix in expected_extensions: | |
| expected_output_file = Path(file_path).with_suffix(".predictions.slp") | |
| assert Path(expected_output_file).exists() | |
| def test_sleap_track_text_file_input( | |
| min_centroid_model_path: str, | |
| min_centered_instance_model_path: str, | |
| centered_pair_vid_path, | |
| tmpdir, | |
| ): | |
| # Create temporary directory with the structured video files | |
| slp_path = Path(tmpdir.mkdir("mp4_directory")) | |
| # Copy and paste the video into the temp dir multiple times | |
| num_copies = 3 | |
| file_paths = [] | |
| for i in range(num_copies): | |
| # Construct the destination path with a unique name | |
| dest_path = slp_path / f"centered_pair_vid_copy_{i}.mp4" | |
| shutil.copy(centered_pair_vid_path, dest_path) | |
| file_paths.append(dest_path) | |
| # Create a text file with the file paths | |
| txt_file_path = slp_path / "file_paths.txt" | |
| with open(txt_file_path, mode="w") as txt_file: | |
| for file_path in file_paths: | |
| txt_file.write(f"{file_path}\n") | |
| slp_path_obj = Path(slp_path) | |
| # Create sleap-track command | |
| args = ( | |
| f"{txt_file_path} --model {min_centroid_model_path} " | |
| f"--tracking.tracker simple " | |
| 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 = available_video_exts() | |
| for file_path in file_paths: | |
| expected_output_file = file_path.with_suffix(".predictions.slp") | |
| assert Path(expected_output_file).exists() |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
tests/nn/test_inference.py (4)
1815-1820: Ensure all expected output files are checked.The current implementation only checks for
.TESTpredictions.slpfiles. Ensure that all expected output files are checked, regardless of the input file extension.- for file_path in slp_path_list: - if file_path.suffix in expected_extensions: - expected_output_file = file_path.with_suffix(".TESTpredictions.slp") - assert Path(expected_output_file).exists() + for file_path in file_paths: + expected_output_file = file_path.with_suffix(".TESTpredictions.slp") + assert Path(expected_output_file).exists()
1848-1851: Ensure specific exception message is checked.Currently, the test only checks for a
ValueErrorexception. Ensure that the specific exception message is checked to verify the correct error is raised.- with pytest.raises( - ValueError, - ): + with pytest.raises(ValueError, match="Expected error message"):
1861-1862: Ensure specific exception message is checked.Currently, the test only checks for a
ValueErrorexception. Ensure that the specific exception message is checked to verify the correct error is raised.- with pytest.raises(ValueError): + with pytest.raises(ValueError, match="Expected error message"):
1905-1910: Ensure all expected output files are checked.The current implementation only checks for
.predictions.slpfiles. Ensure that all expected output files are checked, regardless of the input file extension.- for file_path in slp_path_list: - if file_path.suffix in expected_extensions: - expected_output_file = Path(file_path).with_suffix(".predictions.slp") - assert Path(expected_output_file).exists() + for file_path in file_paths: + expected_output_file = file_path.with_suffix(".predictions.slp") + assert Path(expected_output_file).exists()
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/nn/test_inference.py (8 hunks)
Additional context used
Learnings (1)
tests/nn/test_inference.py (1)
Learnt from: talmo PR: talmolab/sleap#1875 File: tests/nn/test_inference.py:1804-1814 Timestamp: 2024-07-23T23:52:30.705Z Learning: When checking for expected output files in tests, use the supported video file extensions from `sleap.io.video.available_video_exts()` to ensure all formats are covered.
Ruff
tests/nn/test_inference.py
13-13:
tensorflow_hubimported but unusedRemove unused import:
tensorflow_hub(F401)
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/nn/test_inference.py (8 hunks)
Additional context used
Learnings (1)
tests/nn/test_inference.py (1)
Learnt from: talmo PR: talmolab/sleap#1875 File: tests/nn/test_inference.py:1804-1814 Timestamp: 2024-07-23T23:52:30.705Z Learning: When checking for expected output files in tests, use the supported video file extensions from `sleap.io.video.available_video_exts()` to ensure all formats are covered.
Ruff
tests/nn/test_inference.py
13-13:
tensorflow_hubimported but unusedRemove unused import:
tensorflow_hub(F401)
| with pytest.raises(ValueError): | ||
| sleap_track(args=args_empty) |
There was a problem hiding this comment.
Ensure specific exception message is checked.
Currently, the test only checks for a ValueError exception. Ensure that the specific exception message is checked to verify the correct error is raised.
- with pytest.raises(ValueError):
+ with pytest.raises(ValueError, match="Expected error message"):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.
| with pytest.raises(ValueError): | |
| sleap_track(args=args_empty) | |
| with pytest.raises(ValueError, match="Expected error message"): | |
| sleap_track(args=args_empty) |
| expected_extensions = available_video_exts() | ||
|
|
||
| for file_path in slp_path_list: | ||
| if file_path.suffix in expected_extensions: | ||
| expected_output_file = Path(file_path).with_suffix(".predictions.slp") | ||
| assert Path(expected_output_file).exists() |
There was a problem hiding this comment.
Ensure all expected output files are checked.
The current implementation only checks for .predictions.slp files. Ensure that all expected output files are checked, regardless of the input file extension.
- for file_path in slp_path_list:
- if file_path.suffix in expected_extensions:
- expected_output_file = file_path.with_suffix(".predictions.slp")
- assert Path(expected_output_file).exists()
+ for file_path in file_paths:
+ expected_output_file = file_path.with_suffix(".predictions.slp")
+ assert Path(expected_output_file).exists()Committable suggestion was skipped due to low confidence.
| sleap_track(args=args) | ||
|
|
||
|
|
||
| def test_sleap_track_csv_input( |
There was a problem hiding this comment.
Ensure all expected output files are checked.
The current implementation only checks for .TESTpredictions.slp files. Ensure that all expected output files are checked, regardless of the input file extension.
- for file_path in slp_path_list:
- if file_path.suffix in expected_extensions:
- expected_output_file = file_path.with_suffix(".TESTpredictions.slp")
- assert Path(expected_output_file).exists()
+ for file_path in file_paths:
+ expected_output_file = file_path.with_suffix(".TESTpredictions.slp")
+ assert Path(expected_output_file).exists()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_csv_input( | |
| def test_sleap_track_csv_input( | |
| for file_path in file_paths: | |
| expected_output_file = file_path.with_suffix(".TESTpredictions.slp") | |
| assert Path(expected_output_file).exists() |
| # Run inference and expect ValueError for missing 'data_path' column | ||
| with pytest.raises( | ||
| ValueError, | ||
| ): | ||
| sleap_track(args=args_missing_column) |
There was a problem hiding this comment.
Ensure specific exception message is checked.
Currently, the test only checks for a ValueError exception. Ensure that the specific exception message is checked to verify the correct error is raised.
- with pytest.raises(
- ValueError,
- ):
+ with pytest.raises(ValueError, match="Expected error message"):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.
| # Run inference and expect ValueError for missing 'data_path' column | |
| with pytest.raises( | |
| ValueError, | |
| ): | |
| sleap_track(args=args_missing_column) | |
| # Run inference and expect ValueError for missing 'data_path' column | |
| with pytest.raises(ValueError, match="Expected error message"): | |
| sleap_track(args=args_missing_column) |
There was a problem hiding this comment.
looks good!
- Check the test csv function to see if that is the behavior you want now that we are not specifying a column name.
- You can go ahead and add to the CLI documentation.
- Manually test your changes by running the sleap-track command with different arguments to see if you can break it somehow.
Great job!
…ort-to-sleap-track' into elise/add-csv-and-text-file-support-to-sleap-track
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
docs/guides/cli.md (1)
141-144: Inconsistency indata_pathDescriptionThe expanded description for
data_pathis present in one instance but not in others within the same file. To maintain clarity and consistency, please update all instances ofdata_pathto include the expanded description.
- File:
docs/guides/cli.md
- Instances to Update:
usage: sleap-inspect [-h] [--verbose] data_pathusage: sleap-render [-h] [-o OUTPUT] [-f FPS] [--scale SCALE] [--crop CROP] [--frames FRAMES] [--video-index VIDEO_INDEX] data_pathAnalysis chain
LGTM! Verify consistency across the documentation.
The expanded description for
data_pathenhances clarity on acceptable input formats.Ensure that similar updates are made consistently across all relevant documentation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the expanded description for `data_path` is consistent across all relevant documentation. # Test: Search for the `data_path` argument in the documentation. Expect: Consistent descriptions. rg --type md -A 5 $'data_path'Length of output: 2464
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- docs/guides/cli.md (2 hunks)
- sleap/nn/inference.py (8 hunks)
- tests/nn/test_inference.py (8 hunks)
Additional context used
Learnings (1)
tests/nn/test_inference.py (1)
Learnt from: talmo PR: talmolab/sleap#1875 File: tests/nn/test_inference.py:1804-1814 Timestamp: 2024-07-23T23:52:30.705Z Learning: When checking for expected output files in tests, use the supported video file extensions from `sleap.io.video.available_video_exts()` to ensure all formats are covered.
Additional comments not posted (6)
docs/guides/cli.md (1)
159-159: LGTM!The expanded description for
outputenhances the flexibility of the output specification.tests/nn/test_inference.py (2)
1862-1863: Ensure specific exception message is checked.Currently, the test only checks for a
ValueErrorexception. Ensure that the specific exception message is checked to verify the correct error is raised.- with pytest.raises(ValueError): + with pytest.raises(ValueError, match="Expected error message"):Likely invalid or redundant comment.
1848-1852: Ensure specific exception message is checked.Currently, the test only checks for a
ValueErrorexception. Ensure that the specific exception message is checked to verify the correct error is raised.- with pytest.raises( - ValueError, - ): + with pytest.raises(ValueError, match="Expected error message"):Likely invalid or redundant comment.
sleap/nn/inference.py (3)
5306-5307: Initializeoutput_path_listas an empty list.To maintain consistency and avoid potential issues, initialize
output_path_listas an empty list instead ofNone.- output_path_list = None + output_path_list = []
5314-5346: Ensure robust handling of CSV files.The logic for handling CSV files is sound, but consider adding more detailed error messages for better debugging.
- raise ValueError(f"CSV file is empty: {data_path}. Error: {e}") from e + raise ValueError(f"CSV file is empty or invalid: {data_path}. Error: {e}") from e
5401-5409: Improve error handling for video reading.Include the file name in the error message for better debugging.
- except Exception: - print(f"Error reading file: {file_path.as_posix()}") + except Exception as e: + print(f"Error reading file: {file_path.as_posix()}. Error: {e}")
| if file_path.suffix in expected_extensions: | ||
| expected_output_file = Path(file_path).with_suffix(".predictions.slp") | ||
| assert Path(expected_output_file).exists() |
There was a problem hiding this comment.
Ensure all expected output files are checked.
The current implementation only checks for .predictions.slp files. Ensure that all expected output files are checked, regardless of the input file extension.
- for file_path in slp_path_list:
- if file_path.suffix in expected_extensions:
- expected_output_file = Path(file_path).with_suffix(".predictions.slp")
- assert Path(expected_output_file).exists()
+ for file_path in file_paths:
+ expected_output_file = file_path.with_suffix(".predictions.slp")
+ assert Path(expected_output_file).exists()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 file_path.suffix in expected_extensions: | |
| expected_output_file = Path(file_path).with_suffix(".predictions.slp") | |
| assert Path(expected_output_file).exists() | |
| for file_path in file_paths: | |
| expected_output_file = file_path.with_suffix(".predictions.slp") | |
| assert Path(expected_output_file).exists() |
| expected_extensions = available_video_exts() | ||
|
|
||
| for file_path in slp_path_list: | ||
| if file_path.suffix in expected_extensions: | ||
| expected_output_file = file_path.with_suffix(".TESTpredictions.slp") | ||
| assert Path(expected_output_file).exists() |
There was a problem hiding this comment.
Ensure all expected output files are checked.
The current implementation only checks for .TESTpredictions.slp files. Ensure that all expected output files are checked, regardless of the input file extension.
- for file_path in slp_path_list:
- if file_path.suffix in expected_extensions:
- expected_output_file = file_path.with_suffix(".TESTpredictions.slp")
- assert Path(expected_output_file).exists()
+ for file_path in file_paths:
+ expected_output_file = file_path.with_suffix(".TESTpredictions.slp")
+ assert Path(expected_output_file).exists()Committable suggestion was skipped due to low confidence.
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)
- docs/guides/cli.md (2 hunks)
Additional comments not posted (2)
docs/guides/cli.md (2)
141-144: Improved clarity and usability of thedata_pathargument.The expanded description of the
data_pathargument to include multiple input formats significantly improves the clarity and usability of the CLI documentation.
159-159: Enhanced functionality and flexibility of the-o OUTPUTargument.The modified description of the
-o OUTPUTargument to specify that the output can be a filename or a directory path enhances the functionality and flexibility of the CLI.
Description
After having expanded sleap-track to support multiple videos inputs at once, we want to further simplify this process for projects with a large numbers of inputs.
Types of changes
Does this address any currently open issues?
#1870
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
data_pathand the-o OUTPUTargument in the CLI documentation, enhancing user understanding.