Add option to analysis export for exporting predictions for all frames including those with no predictions #1624
Conversation
added all_frames variable to exportAnalysisFile() and exportCSVFile() command definitions (ln 329, 331). added all_frames conditional to ExportAnalysisFile(command) definition
added dropdown menu button options for exporting csvs with all frames (ln 495-521)
WalkthroughThe recent updates to SLEAP focus on enhancing CSV export functionality. New menu items have been added for exporting CSVs with options to include all frames or only tracked ones, across current or all videos. The Changes
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 X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- sleap/gui/app.py (1 hunks)
- sleap/gui/commands.py (2 hunks)
- sleap/info/write_tracking_h5.py (1 hunks)
- sleap/io/format/csv.py (2 hunks)
Files skipped from review due to trivial changes (1)
- sleap/gui/app.py
Additional comments: 5
sleap/gui/commands.py (3)
329-335: The addition of the
all_framesparameter to theexportAnalysisFileandexportCSVFilemethods in theCommandContextclass is consistent with the PR objective to allow exporting of all frames, including those without predictions.1142-1161: The use of the
all_framesparameter in theExportAnalysisFileclass'sdo_actionmethod to control the inclusion of all frames in the export is correctly implemented and aligns with the PR objective.1163-1165: The update to the
askmethod in theExportAnalysisFileclass to handle theall_framesparameter when prompting the user for the filename and other export options is appropriate for the new feature.sleap/info/write_tracking_h5.py (1)
- 348-358: The early return when there are no tracks to export is a good practice to avoid unnecessary processing and potential errors.
sleap/io/format/csv.py (1)
- 63-71: The changes to the
write_analysisfunction call correctly implement the newall_framesparameter, ensuring that the export behavior can be controlled as intended by the PR.
sleap/info/write_tracking_h5.py
Outdated
| tracks.append(detection) | ||
|
|
||
| tracks = pd.DataFrame(tracks) | ||
| tracks.to_csv(output_path, index=False) | ||
|
|
||
| if all_frames: | ||
| tracks = tracks.set_index('frame_idx') | ||
| tracks = tracks.reindex(range(0, len(data_dict['track_occupancy'])),fill_value=0) | ||
| tracks = tracks.reset_index(drop=False) | ||
| tracks.to_csv(output_path, index=False) | ||
| else: | ||
| tracks.to_csv(output_path, index=False) |
There was a problem hiding this comment.
Consider handling the case where valid_frame_idxs is empty, which could occur when all_frames is False and there are no frames with tracking data. This would prevent writing an empty CSV or one with incorrect data.
tests/gui/test_commands.py
Outdated
| for video in all_videos: | ||
| labels.remove_video(labels.videos[-1]) | ||
|
|
||
| params = {"all_videos": True, "csv": csv} | ||
| params = {"all_videos": True, "all_frames": True, "csv": csv} | ||
| with pytest.raises(ValueError): | ||
| okay = ExportAnalysisFile_ask(context=context, params=params) | ||
|
|
There was a problem hiding this comment.
The test case starting at line 341 is attempting to remove all videos and then raise a ValueError when trying to export with all_videos set to True and all_frames set to True. However, the loop at lines 341-342 is removing videos using an index that may not exist after the first iteration, as it always tries to remove the last video in the list. This could lead to an IndexError if the list becomes empty before the loop finishes. A safer approach would be to iterate over a copy of the list or to remove videos without using an index.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- sleap/gui/commands.py (2 hunks)
Additional comments: 3
sleap/gui/commands.py (3)
329-335: The method
exportAnalysisFilehas been updated to include anall_framesparameter, which allows exporting all frames to the CSV file, including those without predictions, when set toTrue.333-335: The method
exportCSVFilehas been updated to include anall_framesparameter, which allows exporting all frames to the CSV file, including those without predictions, when set toTrue.1142-1161: The
writemethod insleap/io/format/csv.pyshould be verified to ensure it correctly handles theall_framesparameter when exporting data to CSV.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- sleap/info/write_tracking_h5.py (1 hunks)
Additional comments: 1
sleap/info/write_tracking_h5.py (1)
- 366-366: The summary states that the default value of
all_frameswas changed fromTruetoFalse, but the code already defaults toFalse. This might be an inconsistency or an outdated summary.
sleap/info/write_tracking_h5.py
Outdated
| if all_frames: | ||
| tracks = tracks.set_index('frame_idx') | ||
| tracks = tracks.reindex(range(0, len(data_dict['track_occupancy'])),fill_value=np.nan) | ||
| tracks = tracks.reset_index(drop=False) | ||
| tracks.to_csv(output_path, index=False) | ||
| else: | ||
| tracks.to_csv(output_path, index=False) |
There was a problem hiding this comment.
The logic to include or exclude frames without predictions based on the all_frames flag is correctly implemented. However, the method of accessing all_frames should be improved for better maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- sleap/gui/commands.py (2 hunks)
Additional comments: 4
sleap/gui/commands.py (4)
329-335: The changes to
exportAnalysisFileandexportCSVFilemethods to include theall_framesparameter align with the PR's objective to enhance CSV export functionality to include rows for all frames. This will improve compatibility with downstream analysis tools.329-331: The
exportAnalysisFilemethod correctly passes theall_framesparameter to theExportAnalysisFilecommand, ensuring that the new functionality is applied when exporting analysis files in non-CSV formats.333-335: The
exportCSVFilemethod correctly passes theall_framesparameter to theExportAnalysisFilecommand withcsv=True, ensuring that the new functionality is applied when exporting analysis files in CSV format.1142-1160: The changes in the
writemethod of theExportAnalysisFilecommand to handle theall_framesparameter are crucial for the new functionality. It ensures that when exporting analysis files, the option to include all frames is respected based on the user's choice.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/app.py (1 hunks)
- sleap/gui/commands.py (2 hunks)
Additional comments: 4
sleap/gui/app.py (2)
496-520: The new menu items for exporting CSV files have been added correctly, with appropriate labels and lambda functions to pass the correct parameters to the
exportCSVFilemethod. The use oflambdaensures that theall_framesandall_videosparameters are set correctly when the menu items are triggered. This aligns with the PR objectives to allow exporting CSV files with all frames or only frames with tracking.496-520: Verify that the
exportCSVFilemethod insleap/gui/commands.pyhas been updated to handle the newall_framesparameter.
#!/bin/bash # Corrected search for the `exportCSVFile` method definition to check if it handles the `all_frames` parameter. ast-grep --lang python --pattern $'def exportCSVFile($_, $_, all_frames: bool = False) { $$$ }'sleap/gui/commands.py (2)
329-335: The addition of the
all_framesparameter to theexportAnalysisFileandexportCSVFilemethods aligns with the PR objectives to enhance CSV export functionality for compatibility with behavioral classification pipelines.1146-1161: The implementation of the
all_framesparameter in thedo_actionmethod of theExportAnalysisFileclass correctly passes this parameter to theadaptor.writemethod, ensuring that the exported file includes all frames when requested.
sleap/gui/commands.py
Outdated
| if params['all_frames']: | ||
| adaptor.write( | ||
| filename=output_path, | ||
| all_frames=True, | ||
| source_object=context.labels, | ||
| source_path=context.state["filename"], | ||
| video=video, | ||
| ) | ||
|
|
||
| ) | ||
| else: | ||
| adaptor.write( | ||
| filename=output_path, | ||
| all_frames=False, | ||
| source_object=context.labels, | ||
| source_path=context.state["filename"], | ||
| video=video, | ||
| ) |
There was a problem hiding this comment.
Consider refactoring the do_action method in the ExportAnalysisFile class to reduce code duplication by calling adaptor.write once and setting the all_frames parameter based on the condition.
- if params['all_frames']:
- adaptor.write(
- filename=output_path,
- all_frames=True,
- source_object=context.labels,
- source_path=context.state["filename"],
- video=video,
- )
- else:
- adaptor.write(
- filename=output_path,
- all_frames=False,
- source_object=context.labels,
- source_path=context.state["filename"],
- video=video,
- )
+ adaptor.write(
+ filename=output_path,
+ all_frames=params['all_frames'],
+ source_object=context.labels,
+ source_path=context.state["filename"],
+ video=video,
+ )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 params['all_frames']: | |
| adaptor.write( | |
| filename=output_path, | |
| all_frames=True, | |
| source_object=context.labels, | |
| source_path=context.state["filename"], | |
| video=video, | |
| ) | |
| ) | |
| else: | |
| adaptor.write( | |
| filename=output_path, | |
| all_frames=False, | |
| source_object=context.labels, | |
| source_path=context.state["filename"], | |
| video=video, | |
| ) | |
| adaptor.write( | |
| filename=output_path, | |
| all_frames=params['all_frames'], | |
| source_object=context.labels, | |
| source_path=context.state["filename"], | |
| video=video, | |
| ) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (6)
- sleap/gui/app.py (1 hunks)
- sleap/gui/commands.py (2 hunks)
- sleap/info/write_tracking_h5.py (1 hunks)
- sleap/io/format/csv.py (2 hunks)
- sleap/io/format/nix.py (1 hunks)
- sleap/io/format/sleap_analysis.py (1 hunks)
Additional comments: 8
sleap/gui/app.py (1)
- 496-537: The changes to the menu items in the
_create_menusmethod correctly implement the new functionality for exporting CSV files with options for including all frames. Ensure that theexportCSVFilemethod insleap/gui/commands.pyis updated to handle the newall_framesparameter as intended.sleap/gui/commands.py (4)
329-335: The addition of the
all_framesparameter to theexportAnalysisFileandexportCSVFilemethods aligns with the PR's objective to enhance CSV export functionality, allowing users to include rows for all frames in the exported files.1146-1161: Ensure that the
writemethod in theadaptorclasses being used supports theall_framesparameter and handles it correctly to include or exclude frames without tracking data as intended.
#!/bin/bash # Correcting the script to use the proper flag --after-context to verify the `write` method in adaptor classes. rg --multiline 'def write\(' --after-context=20 | rg 'all_frames'
- 1163-1165: Ensure that the
ask_for_filenamefunction is defined within theExportAnalysisFileclass and correctly uses theall_framesparameter when prompting the user for a filename.
#!/bin/bash # Correcting the previous script to use the proper flag for context lines after the match rg 'def ask_for_filename\(' --after-context=20 | rg 'all_frames'
- 1163-1165: Ensure that the
ask_for_filenamefunction is defined within theExportAnalysisFileclass and correctly uses theall_framesparameter when prompting the user for a filename.
#!/bin/bash # Correcting the previous script to use the proper flag for ripgrep. rg 'def ask_for_filename\(' --after-context=20 | rg 'all_frames'sleap/info/write_tracking_h5.py (2)
366-366: The change of the default value for
all_framesfromTruetoFalsein themainfunction aligns with the PR objective to make the inclusion of all frames optional.353-357: Verify that the
frame_idxcolumn exists in thetracksDataFrame and that the range used for reindexing (range(0, len(data_dict['track_occupancy']))) correctly represents all frame indices in the context of the data.
Verify that the
frame_idxcolumn exists in thetracksDataFrame and that the range used for reindexing (range(0, len(data_dict['track_occupancy']))) correctly represents all frame indices in the context of the data.sleap/io/format/csv.py (1)
- 57-58: The docstring has been correctly updated to describe the new
all_framesparameter.
| all_frames: bool = False, | ||
| video: Video = None, |
There was a problem hiding this comment.
The write function has been updated to include a new all_frames parameter, which is consistent with the PR's objective to allow exporting data with rows for all frames. However, the docstring for the write function has not been updated to include the new parameter. It is important to maintain accurate documentation, so the docstring should be updated to describe the all_frames parameter and its effect on the function's behavior.
sleap/info/write_tracking_h5.py
Outdated
| @@ -348,14 +348,22 @@ def write_csv_file(output_path, data_dict): | |||
| tracks.append(detection) | |||
|
|
|||
| tracks = pd.DataFrame(tracks) | |||
| tracks.to_csv(output_path, index=False) | |||
| all_frames = globals().get('all_frames', False) | |||
There was a problem hiding this comment.
The use of globals().get('all_frames', False) to retrieve the all_frames variable is not a standard practice and can lead to unexpected behavior. It would be safer and more maintainable to pass all_frames as a parameter to the write_csv_file function.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- sleap/gui/app.py (1 hunks)
- sleap/gui/commands.py (2 hunks)
- sleap/info/write_tracking_h5.py (3 hunks)
- sleap/io/format/csv.py (2 hunks)
- tests/gui/test_commands.py (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/gui/test_commands.py
Additional comments: 13
sleap/gui/app.py (2)
496-521: The addition of menu items for exporting CSV files with options to include all frames or only tracked frames is implemented correctly and aligns with the PR objectives.
514-514: The lambdas used for the new menu items correctly pass the
all_framesparameter to theexportCSVFilemethod, ensuring the new functionality is accessible from the GUI.Also applies to: 520-520
sleap/gui/commands.py (4)
- 326-338: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [329-361]
The implementation of the
all_framesparameter in theexportAnalysisFileandexportCSVFilemethods correctly passes this parameter to theadaptor.writemethod, allowing users to export data with rows for all frames when set toTrue. This change aligns with the PR objective to enhance SLEAP analysis export functionality.
1142-1161: The logic in the
askmethod of theExportAnalysisFileclass correctly prompts the user for the filename and sets theanalysis_videosparameter based on the user's input and theall_videosparameter. This ensures that the export functionality is user-friendly and flexible, allowing for the export of either the current video or all videos in the project.1145-1161: The use of the
all_framesparameter in thewritemethod of theCSVAdaptorandSleapAnalysisAdaptorclasses is consistent with the PR objective. The parameter is used to conditionally reindex thetracksDataFrame before writing to a CSV file, ensuring that rows for all frames are included whenall_framesis set toTrue.1145-1161: The handling of the
all_framesparameter in thedo_actionmethod of theExportAnalysisFileclass is correct. The parameter is checked and passed to theadaptor.writemethod appropriately, allowing the inclusion of all frames in the export based on the user's choice.sleap/info/write_tracking_h5.py (3)
- 348-368: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [290-358]
The
write_csv_filefunction has been updated to include anall_framesparameter which is used to determine whether to include rows for all frames or only those with tracking data when exporting to CSV. The logic within the function has been modified to reindex the DataFrame based on this parameter before writing to a CSV file. This change aligns with the PR's objective to allow exporting data with rows for all frames, filling frames with no predictions with zeros. The implementation appears to handle the DataFrame reindexing correctly based on theall_framesparameter.
365-365: The default value of the
all_framesparameter in themainfunction has been changed fromTruetoFalse. This alters the default behavior of the script and could impact existing workflows. It's important to ensure that this change is clearly communicated to users and that all documentation and related code are updated to reflect this new default.445-445: The
write_csv_filefunction call in themainfunction has been updated to pass theall_framesparameter explicitly. This change is necessary to ensure that the user's choice regarding frame inclusion is respected when exporting to CSV.sleap/io/format/csv.py (4)
48-48: The addition of the
all_framesparameter to thewritefunction aligns with the PR objectives to allow exporting data with rows for all frames.57-58: The docstring for the
all_framesparameter clearly explains its purpose, which is to include all frames or only those with tracking data in the export.69-69: The
all_framesparameter is correctly passed to thewrite_analysisfunction, ensuring the new functionality is utilized.54-72: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [45-72]
Verify that existing calls to the
writefunction do not require updates to explicitly set the newall_framesparameter where needed.
| tracks.append(detection) | ||
|
|
||
| tracks = pd.DataFrame(tracks) | ||
| tracks.to_csv(output_path, index=False) | ||
|
|
||
| if all_frames: | ||
| tracks = tracks.set_index('frame_idx') | ||
| tracks = tracks.reindex(range(0, len(data_dict['track_occupancy'])), fill_value=np.nan) | ||
| tracks = tracks.reset_index(drop=False) | ||
| tracks.to_csv(output_path, index=False) | ||
| else: | ||
| tracks.to_csv(output_path, index=False) | ||
|
|
||
|
|
||
| def main( | ||
| labels: Labels, | ||
| output_path: str, | ||
| labels_path: str = None, | ||
| all_frames: bool = True, | ||
| all_frames: bool = False, | ||
| video: Video = None, | ||
| csv: bool = False, | ||
| ): |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [361-447]
Consider adding error handling in the main function to manage potential exceptions that could be raised during the execution of the script. This would improve the robustness and user experience by providing more informative error messages and handling edge cases gracefully.
Description
Previously, SLEAP-exported analysis csvs only included rows in which there was a predicted instance, meaning imports into downstream behavioral classification pipelines (e.g. SimBA), lack rows for frames that are missing tracking predictions. These set of changes to
write_tracking_h5.pyand the GUI add an option to the csv export menu to export with rows for all frames (frames with no predictions will be filled with 0).Location of added buttons below:
Types of changes
Does this address any currently open issues?
#1622
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
Enhancements