Add option to export to CSV via sleap-convert and API #1730
Conversation
WalkthroughThis update introduces the capability to export analysis results in CSV format, enhancing the SLEAP project's data exporting functionalities. It streamlines the process for users needing CSV outputs for their tracking information, catering to broader analysis and integration needs, such as compatibility with external tools. Changes
Possibly related issues
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 Configration 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 Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/io/convert.py (4 hunks)
- sleap/io/dataset.py (1 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional comments not posted (3)
sleap/io/convert.py (3)
73-73: Consider adding a validation check for the 'analysis.csv' format to ensure that the necessary conditions for CSV export are met before proceeding with the conversion.
169-180: Ensure that thewrite_analysisfunction called for CSV export properly handles errors and exceptions, especially for file I/O operations.
165-184: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [73-180]
Overall, the integration of the CSV export functionality appears well-implemented. However, consider adding unit tests to verify the correctness and robustness of the new feature.
Would you like assistance in creating unit tests for the CSV export functionality?
sleap/io/convert.py
Outdated
| @@ -105,6 +106,7 @@ def main(args: list = None): | |||
| """ | |||
| parser = create_parser() | |||
| args = parser.parse_args(args=args) | |||
| print(args) | |||
There was a problem hiding this comment.
Remove the debug print statement to maintain a clean and professional codebase.
- print(args)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.
| print(args) |
| if "nix" in args.format: | ||
| out_suffix = "nix" | ||
| elif "csv" in args.format: | ||
| out_suffix = "csv" | ||
| else: | ||
| out_suffix = "h5" |
There was a problem hiding this comment.
Refactor the logic for determining the output file suffix into a separate function to improve code readability and maintainability.
+ def determine_output_suffix(format_arg: str) -> str:
+ if "nix" in format_arg:
+ return "nix"
+ elif "csv" in format_arg:
+ return "csv"
+ else:
+ return "h5"
- if "nix" in args.format:
- out_suffix = "nix"
- elif "csv" in args.format:
- out_suffix = "csv"
- else:
- out_suffix = "h5"
+ out_suffix = determine_output_suffix(args.format)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.
| if "nix" in args.format: | |
| out_suffix = "nix" | |
| elif "csv" in args.format: | |
| out_suffix = "csv" | |
| else: | |
| out_suffix = "h5" | |
| def determine_output_suffix(format_arg: str) -> str: | |
| if "nix" in format_arg: | |
| return "nix" | |
| elif "csv" in format_arg: | |
| return "csv" | |
| else: | |
| return "h5" | |
| out_suffix = determine_output_suffix(args.format) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1730 +/- ##
===========================================
+ Coverage 73.30% 74.12% +0.81%
===========================================
Files 134 134
Lines 24087 24456 +369
===========================================
+ Hits 17658 18127 +469
+ Misses 6429 6329 -100 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (11)
sleap/io/dataset.py (11)
Line range hint
55-55: Remove duplicate import ofCallable.- from typing import CallableThe
Callabletype is imported twice, which is unnecessary. Consider removing the duplicate import to clean up the code.
Line range hint
61-61: Remove unused importh5py.- import h5py as h5The
h5pymodule is imported but not used in the provided code. Removing unused imports helps to keep the codebase clean and maintainable.
Line range hint
68-68: Replace bareexceptwith specific exceptions.Using a bare
exceptcan catch unexpected exceptions, making debugging more difficult. It's better to catch specific exceptions to handle known error conditions.
Line range hint
69-69: Remove unused import_ForwardRef.- from typing import _ForwardRef as ForwardRef + passThe
_ForwardRefimport is unused in the provided code. Consider removing it to clean up the code.
Line range hint
304-314: Refactor lambda expressions to named functions.Using named functions instead of lambda expressions for complex logic can improve readability and maintainability.
- filter_func = lambda lf: video is None or lf.video == video + def filter_func(lf): + return video is None or lf.video == videoApply similar changes for other lambda expressions in this range.
Line range hint
854-854: Useisinstance()for type checks.Type checks should use
isinstance()for better readability and flexibility.- if type(key) != tuple: + if not isinstance(key, tuple):
Line range hint
2209-2209: Add missing import forglob.+ from glob import globThe
globfunction is used but not imported. Adding the missing import will resolve theUndefined name 'glob'issue.
Line range hint
2304-2304: Refine conditional check to useif not ret:.- if ret == False: + if not ret:Using
if not ret:is more Pythonic and concise for checking false conditions.
Line range hint
2407-2407: Define or importsleapfor theto_pipelinemethod.The
sleapnamespace is referenced but not defined or imported in the provided context. Ensure thatsleapis correctly imported or defined to avoid runtime errors.
Line range hint
2500-2500: Remove unused variablee.- except IndexError as e: + except IndexError:The variable
eis assigned in theexceptblock but never used. Removing it will clean up the code.
Line range hint
2502-2502: Correct the f-string without placeholders.- f"There are no videos in this project. No points matrix to return." + "There are no videos in this project. No points matrix to return."The f-string is used without any placeholders. Converting it to a regular string is more appropriate.
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/io/convert.py (3 hunks)
- sleap/io/dataset.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/io/convert.py
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (1)
tests/io/test_convert.py (1)
Line range hint
100-100: Address unused variablelabels.The static analysis has identified that the local variable
labelsis assigned to but never used within thetest_sleap_formatfunction. This could potentially be an oversight or leftover from previous iterations of the code. Consider removing or utilizing this variable as appropriate to clean up the code and avoid confusion.- labels = min_labels_slp
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/io/test_convert.py (2 hunks)
Additional comments not posted (2)
tests/io/test_convert.py (2)
11-11: Add test cases for new CSV export functionality.The inclusion of
"analysis.csv"in the@pytest.mark.parametrizedecorator effectively extends the test coverage to include the new CSV export functionality. This is a positive change that ensures the feature is tested across its intended formats.
30-30: Refine logic for determining output file format.The updated logic in the
generate_filenamesfunction to prioritize"csv"over"nix"and"h5"based on theformatparameter is clear and aligns with the new feature's requirements. This change ensures that the correct file format is used for output files when exporting to CSV.
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (5)
tests/io/test_dataset.py (5)
Line range hint
13-13: The importsleap.io.legacy.load_labels_json_oldappears to be unused in this file.- from sleap.io.legacy import load_labels_json_old
Line range hint
117-119: Useis notfor object identity tests andnot infor membership tests to adhere to Python best practices.- assert not multi_skel_vid_labels.skeletons[0] is loaded_labels.skeletons[0] + assert multi_skel_vid_labels.skeletons[0] is not loaded_labels.skeletons[0] - assert not multi_skel_vid_labels.nodes[3] in loaded_labels.nodes + assert multi_skel_vid_labels.nodes[3] not in loaded_labels.nodes - assert not multi_skel_vid_labels.videos[0] is loaded_labels.videos[0] + assert multi_skel_vid_labels.videos[0] is not loaded_labels.videos[0]
Line range hint
690-690: The variablenew_labels_jsonis assigned but never used. Consider removing it if it's unnecessary.- new_labels_json = new_labels.to_dict()
Line range hint
706-706: Similar to a previous comment, the variablenew_labels_jsonis assigned but not used. Removing unnecessary variable assignments can improve code clarity.- new_labels_json = new_labels.to_dict()
Line range hint
1423-1423: The variabletrack_instsis assigned but never used. If the intention was to perform assertions or checks using this variable, ensure those are implemented. Otherwise, consider removing the assignment.- track_insts = [inst for inst in labels.instances() if inst.track == track]
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/io/test_dataset.py (2 hunks)
Additional comments not posted (3)
tests/io/test_dataset.py (3)
2-2: Consider grouping standard library imports together for better readability.
8-8: The addition ofget_nodes_as_np_stringsimport aligns with the new functionality for exporting to CSV. Ensure it's utilized effectively in the new test functions.
1573-1603: The new test functions for exporting to CSV files (test_export_csvandtest_exported_csv) are crucial for verifying the new CSV export functionality. Ensure comprehensive test coverage, including edge cases and error handling, to maintain robustness.
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (7)
tests/io/test_dataset.py (7)
Line range hint
13-13: Remove the unused import to clean up the code.- from sleap.io.legacy import load_labels_json_old
Line range hint
117-117: Useis notfor object identity tests.- assert not multi_skel_vid_labels.skeletons[0] is loaded_labels.skeletons[0] + assert multi_skel_vid_labels.skeletons[0] is not loaded_labels.skeletons[0]
Line range hint
118-118: Usenot infor membership tests.- assert not multi_skel_vid_labels.nodes[3] in loaded_labels.nodes + assert multi_skel_vid_labels.nodes[3] not in loaded_labels.nodes
Line range hint
119-119: Useis notfor object identity tests.- assert not multi_skel_vid_labels.videos[0] is loaded_labels.videos[0] + assert multi_skel_vid_labels.videos[0] is not loaded_labels.videos[0]
Line range hint
1423-1423: Remove the unused variable to clean up the code.- track_insts = [inst for inst in labels.instances() if inst.track == track]
Line range hint
690-690: The variablenew_labels_jsonis assigned but never used. Consider removing it if it's not needed.- new_labels_json = new_labels.to_dict()
Line range hint
706-706: The variablenew_labels_jsonis assigned but never used. Consider removing it if it's not needed.- new_labels_json = new_labels.to_dict()
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/io/test_dataset.py (2 hunks)
Additional comments not posted (1)
tests/io/test_dataset.py (1)
1576-1605: Ensure the CSV export tests verify the content of the CSV files.Consider adding assertions to check the actual content of the exported CSV files against expected values to ensure the export functionality works as intended.
There was a problem hiding this comment.
LGTM, but can you also update the relevant part of the docs?
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (7)
tests/io/test_dataset.py (7)
Line range hint
13-13: Remove unused importsleap.io.legacy.load_labels_json_old.- import sleap.io.legacy.load_labels_json_old
Line range hint
117-117: Useis notfor object identity checks.- assert not multi_skel_vid_labels.skeletons[0] is loaded_labels.skeletons[0] + assert multi_skel_vid_labels.skeletons[0] is not loaded_labels.skeletons[0]
Line range hint
118-118: Usenot infor membership checks.- assert not multi_skel_vid_labels.nodes[3] in loaded_labels.nodes + assert multi_skel_vid_labels.nodes[3] not in loaded_labels.nodes
Line range hint
119-119: Useis notfor object identity checks.- assert not multi_skel_vid_labels.videos[0] is loaded_labels.videos[0] + assert multi_skel_vid_labels.videos[0] is not loaded_labels.videos[0]
Line range hint
690-690: Remove the unused variable assignment.- new_labels_json = new_labels.to_dict()
Line range hint
706-706: Remove the unused variable assignment.- new_labels_json = new_labels.to_dict()
Line range hint
1423-1423: Remove the unused variable assignment.- track_insts = [inst for inst in labels.instances() if inst.track == track]
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- docs/guides/cli.md (1 hunks)
- tests/io/test_dataset.py (2 hunks)
Additional comments not posted (3)
docs/guides/cli.md (1)
325-326: Consider clarifying the CSV format's capabilities or limitations.It would be helpful for users if the documentation included a brief description of what the CSV format includes or any limitations it might have compared to other formats. This could guide users in choosing the most appropriate format for their needs.
tests/io/test_dataset.py (2)
1576-1587: The test for CSV export functionality is correctly implemented.
1590-1602: The test for verifying the exported CSV content is correctly implemented.
Description
analysis.csvas an option for exporting a csv usingsleap-convertCLIexport_csvfunction toLabelsclass for exporting a csv using the APITypes of changes
Does this address any currently open issues?
#1713
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit