-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to analysis export for exporting predictions for all frames including those with no predictions #1624
base: develop
Are you sure you want to change the base?
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_frames
parameter to theexportAnalysisFile
andexportCSVFile
methods in theCommandContext
class is consistent with the PR objective to allow exporting of all frames, including those without predictions.1142-1161: The use of the
all_frames
parameter in theExportAnalysisFile
class'sdo_action
method 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
ask
method in theExportAnalysisFile
class to handle theall_frames
parameter 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_analysis
function call correctly implement the newall_frames
parameter, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
exportAnalysisFile
has been updated to include anall_frames
parameter, which allows exporting all frames to the CSV file, including those without predictions, when set toTrue
.333-335: The method
exportCSVFile
has been updated to include anall_frames
parameter, which allows exporting all frames to the CSV file, including those without predictions, when set toTrue
.1142-1161: The
write
method insleap/io/format/csv.py
should be verified to ensure it correctly handles theall_frames
parameter when exporting data to CSV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_frames
was changed fromTrue
toFalse
, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
exportAnalysisFile
andexportCSVFile
methods to include theall_frames
parameter 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
exportAnalysisFile
method correctly passes theall_frames
parameter to theExportAnalysisFile
command, ensuring that the new functionality is applied when exporting analysis files in non-CSV formats.333-335: The
exportCSVFile
method correctly passes theall_frames
parameter to theExportAnalysisFile
command withcsv=True
, ensuring that the new functionality is applied when exporting analysis files in CSV format.1142-1160: The changes in the
write
method of theExportAnalysisFile
command to handle theall_frames
parameter 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
exportCSVFile
method. The use oflambda
ensures that theall_frames
andall_videos
parameters 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
exportCSVFile
method insleap/gui/commands.py
has been updated to handle the newall_frames
parameter.
#!/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_frames
parameter to theexportAnalysisFile
andexportCSVFile
methods aligns with the PR objectives to enhance CSV export functionality for compatibility with behavioral classification pipelines.1146-1161: The implementation of the
all_frames
parameter in thedo_action
method of theExportAnalysisFile
class correctly passes this parameter to theadaptor.write
method, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_menus
method correctly implement the new functionality for exporting CSV files with options for including all frames. Ensure that theexportCSVFile
method insleap/gui/commands.py
is updated to handle the newall_frames
parameter as intended.sleap/gui/commands.py (4)
329-335: The addition of the
all_frames
parameter to theexportAnalysisFile
andexportCSVFile
methods 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
write
method in theadaptor
classes being used supports theall_frames
parameter 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_filename
function is defined within theExportAnalysisFile
class and correctly uses theall_frames
parameter 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_filename
function is defined within theExportAnalysisFile
class and correctly uses theall_frames
parameter 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_frames
fromTrue
toFalse
in themain
function aligns with the PR objective to make the inclusion of all frames optional.353-357: Verify that the
frame_idx
column exists in thetracks
DataFrame 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_idx
column exists in thetracks
DataFrame 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_frames
parameter.
all_frames: bool = False, | ||
video: Video = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_frames
parameter to theexportCSVFile
method, 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_frames
parameter in theexportAnalysisFile
andexportCSVFile
methods correctly passes this parameter to theadaptor.write
method, 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
ask
method of theExportAnalysisFile
class correctly prompts the user for the filename and sets theanalysis_videos
parameter based on the user's input and theall_videos
parameter. 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_frames
parameter in thewrite
method of theCSVAdaptor
andSleapAnalysisAdaptor
classes is consistent with the PR objective. The parameter is used to conditionally reindex thetracks
DataFrame before writing to a CSV file, ensuring that rows for all frames are included whenall_frames
is set toTrue
.1145-1161: The handling of the
all_frames
parameter in thedo_action
method of theExportAnalysisFile
class is correct. The parameter is checked and passed to theadaptor.write
method 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_file
function has been updated to include anall_frames
parameter 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_frames
parameter.
365-365: The default value of the
all_frames
parameter in themain
function has been changed fromTrue
toFalse
. 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_file
function call in themain
function has been updated to pass theall_frames
parameter 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_frames
parameter to thewrite
function aligns with the PR objectives to allow exporting data with rows for all frames.57-58: The docstring for the
all_frames
parameter clearly explains its purpose, which is to include all frames or only those with tracking data in the export.69-69: The
all_frames
parameter is correctly passed to thewrite_analysis
function, 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
write
function do not require updates to explicitly set the newall_frames
parameter 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.py
and 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:
![image](https://private-user-images.githubusercontent.com/87097162/287795700-deb82f94-8aeb-41ba-af90-d7d9b49b7409.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ2NjI5NDgsIm5iZiI6MTcyNDY2MjY0OCwicGF0aCI6Ii84NzA5NzE2Mi8yODc3OTU3MDAtZGViODJmOTQtOGFlYi00MWJhLWFmOTAtZDdkOWI0OWI3NDA5LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI2VDA4NTcyOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTExZjI1ZmJhNzA2YWQ5M2FlOTcxNjE1MmZiNTBhYmVjOTY4YmRjMmEyMDJiMzcwMDkzYWFlNWIxOWRkMTFiNDAmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.xS9wJcPGLmGdTM3dEEszTvaHQ9Xeophz-xRCfkZ32x0)
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