Labels.split and Labels.make_training_splits
#98
Conversation
|
Warning Review failedThe pull request is closed. WalkthroughThis release includes enhancements and new features across several modules. Key changes involve updates to file saving and embedding functionalities, introduction of data splitting methods, and improvements in metadata handling. Notable additions include more flexible embedding options when saving labels, methods to create training/validation/test splits, and provenance tracking in label reading functions. These updates aim to enhance user experience, improve data handling, and provide greater control over dataset management. 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 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: 5
Outside diff range and nitpick comments (26)
mkdocs.yml (3)
Line range hint
72-72: Remove trailing spaces to maintain clean and consistent code formatting.Tools
yamllint
[warning] 154-154: wrong indentation: expected 6 but found 4 (indentation)
[error] 156-156: no new line character at the end of file (new-line-at-end-of-file)
Line range hint
101-101: Remove the duplicate keyanchor_linenumsto avoid confusion and potential errors in markdown rendering.- anchor_linenums: trueTools
yamllint
[warning] 154-154: wrong indentation: expected 6 but found 4 (indentation)
[error] 156-156: no new line character at the end of file (new-line-at-end-of-file)
Line range hint
120-120: Remove trailing spaces to maintain clean and consistent code formatting.Also applies to: 138-138
Tools
yamllint
[warning] 154-154: wrong indentation: expected 6 but found 4 (indentation)
[error] 156-156: no new line character at the end of file (new-line-at-end-of-file)
docs/index.md (1)
45-45: Remove multiple consecutive blank lines to comply with markdown best practices and maintain readability.Also applies to: 66-66, 81-81, 82-82, 124-124, 143-143, 158-158, 178-178
Tools
Markdownlint
45-45: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank linessleap_io/io/labelstudio.py (2)
Line range hint
36-36: Consider removing the"r"mode fromopenas it's the default mode.- with open(labels_path, "r") as task_file: + with open(labels_path) as task_file:
Line range hint
39-39: Useisinstancefor type checking instead of direct type comparison for better practice and future Python compatibility.- if type(skeleton) == list: + if isinstance(skeleton, list):tests/io/test_slp.py (1)
Line range hint
326-329: Combine similarifbranches using logicalorto simplify the code.- elif to_embed == "user": - assert labels.video.backend.embedded_frame_inds == [0, 220, 440, 770, 990] - elif to_embed == "suggestions": - assert len(labels.video.backend.embedded_frame_inds) == 10 - elif to_embed == "suggestions+user": - assert len(labels.video.backend.embedded_frame_inds) == 10 + elif to_embed in ["user", "suggestions", "suggestions+user"]: + assert len(labels.video.backend.embedded_frame_inds) == 10sleap_io/io/nwb.py (1)
Line range hint
54-54: Optimize dictionary key access by removing.keys()which is unnecessary and less efficient.- video_keys: List[str] = [key for key in nwb_file.keys() if "SLEAP_VIDEO" in key] + video_keys: List[str] = [key for key in nwb_file if "SLEAP_VIDEO" in key]tests/model/test_labels.py (4)
Line range hint
3-3: Remove unused import:numpy.testing.assert_equal.- from numpy.testing import assert_equal
Line range hint
14-14: Remove unused import:sleap_io.load_video.- from sleap_io import load_video
421-525: The test cases for thesplitfunction are comprehensive and cover various edge cases, including empty data, real data, rounding errors, and serialization. However, consider adding assertions to verify the contents of the splits, not just their lengths, to ensure the data integrity is maintained after the split.
560-577: Thetest_make_training_splits_savefunction tests the saving of labels after making training splits and verifies their integrity post-save. It's well-implemented and checks theprovenancefield to ensure traceability. However, consider adding more detailed checks on the contents of the saved files to ensure all data is correctly serialized and deserialized.sleap_io/io/jabs.py (6)
Line range hint
71-71: Update the type annotation to use modern Python syntax.- def read_labels(labels_path: str, skeleton: Optional[Skeleton] = JABS_DEFAULT_SKELETON) -> Labels: + def read_labels(labels_path: str, skeleton: Skeleton | None = JABS_DEFAULT_SKELETON) -> Labels:
Line range hint
86-86: Modernize the type annotation by usinglistinstead ofList.- frames: List[LabeledFrame] = [] + frames: list[LabeledFrame] = []
Line range hint
141-141: Simplify dictionary key checks by removing.keys().- if "static_objects" in pose_file.keys() + if "static_objects" in pose_fileAlso applies to: 155-155, 537-537
Line range hint
193-193: Update the type annotation to use modern Python syntax.- data: Union[np.ndarray[np.uint16], np.ndarray[np.float32]], + data: np.ndarray[np.uint16] | np.ndarray[np.float32],
Line range hint
229-229: Modernize the type annotation by usinglistinstead ofList.- def get_max_ids_in_video(labels: List[Labels], key: str = "Mouse") -> int: + def get_max_ids_in_video(labels: list[Labels], key: str = "Mouse") -> int:
Line range hint
291-291: Consider adding an explicitstacklevelto warnings to improve debugging.- warnings.warn(f"Skipping {out_filename} because it already exists.") + warnings.warn(f"Skipping {out_filename} because it already exists.", stacklevel=2)Also applies to: 306-306, 343-343
sleap_io/io/video.py (6)
Line range hint
16-19: Consider usingcontextlib.suppressfor cleaner exception handling.- try: - import cv2 - except ImportError: - pass + from contextlib import suppress + with suppress(ImportError): + import cv2
Line range hint
21-24: Refactor repeated try-except blocks for imports usingcontextlib.suppress.- try: - import imageio_ffmpeg - except ImportError: - pass - try: - import av - except ImportError: - pass + from contextlib import suppress + with suppress(ImportError): + import imageio_ffmpeg + with suppress(ImportError): + import avAlso applies to: 26-29
Line range hint
22-22: Imports are unused; consider checking for module availability instead of importing.Consider using
importlib.util.find_specto check for module availability without importing them, which can be more efficient and clear.from importlib import util imageio_ffmpeg_available = util.find_spec("imageio_ffmpeg") is not None av_available = util.find_spec("av") is not NoneAlso applies to: 27-27
Line range hint
55-65: Update type annotations to use modern Python type hinting syntax.- filename: str | Path | list[str] | list[Path] - grayscale: Optional[bool] = None - keep_open: bool = True - _cached_shape: Optional[Tuple[int, int, int, int]] = None - _open_reader: Optional[object] = None + filename: str | Path | list[str | Path] + grayscale: bool | None = None + keep_open: bool = True + _cached_shape: tuple[int, int, int, int] | None = None + _open reader: object | None = NoneAlso applies to: 87-90, 161-167, 486-493, 559-559
Line range hint
528-530: Simplify nestedifstatements.- if "format" in ds.attrs: - if "frame_numbers" in ds.parent: - frame_numbers = ds.parent["frame_numbers"][:].astype(int) - self.frame_map = {frame: idx for idx, frame in enumerate(frame_numbers)} - self.source_inds = frame_numbers + if "format" in ds.attrs and "frame_numbers" in ds.parent: + frame_numbers = ds.parent["frame_numbers"][:].astype(int) + self.frame_map = {frame: idx for idx, frame in enumerate(frame_numbers)} + self.source_inds = frame_numbers
Line range hint
577-580: Use a ternary operator for conditional assignment.- if self.frame_map: - frame_idx = list(self.frame_map.keys())[0] - else: - frame_idx = 0 + frame_idx = list(self.frame_map.keys())[0] if self.frame_map else 0sleap_io/io/slp.py (2)
Line range hint
33-36: Consider usingcontextlib.suppressfor cleaner handling of optional imports.- try: - import cv2 - except ImportError: - pass + from contextlib import suppress + with suppress(ImportError): + import cv2
Line range hint
784-784: For type annotations, use the modern|syntax for union types.- def read_instances(labels_path: str, skeletons: list[Skeleton], tracks: list[Track], points: list[Point], pred_points: list[PredictedPoint], format_id: float) -> list[Union[Instance, PredictedInstance]]: + def read_instances(labels_path: str, skeletons: list[Skeleton], tracks: list[Track], points: list[Point], pred_points: list[PredictedPoint], format_id: float) -> list[Instance | PredictedInstance]:
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- docs/index.md (4 hunks)
- mkdocs.yml (1 hunks)
- sleap_io/io/jabs.py (1 hunks)
- sleap_io/io/labelstudio.py (1 hunks)
- sleap_io/io/main.py (2 hunks)
- sleap_io/io/nwb.py (1 hunks)
- sleap_io/io/slp.py (5 hunks)
- sleap_io/io/video.py (1 hunks)
- sleap_io/model/labels.py (3 hunks)
- sleap_io/model/video.py (2 hunks)
- sleap_io/version.py (1 hunks)
- tests/io/test_slp.py (4 hunks)
- tests/model/test_labels.py (1 hunks)
Files not reviewed due to errors (1)
- sleap_io/model/labels.py (no review received)
Files skipped from review due to trivial changes (1)
- sleap_io/version.py
Additional context used
yamllint
mkdocs.yml
[error] 72-72: trailing spaces (trailing-spaces)
[error] 101-101: duplication of key "anchor_linenums" in mapping (key-duplicates)
[error] 120-120: trailing spaces (trailing-spaces)
[error] 138-138: trailing spaces (trailing-spaces)
[warning] 154-154: wrong indentation: expected 6 but found 4 (indentation)
[error] 156-156: no new line character at the end of file (new-line-at-end-of-file)
Markdownlint
docs/index.md
45-45: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
66-66: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
81-81: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
82-82: Expected: 1; Actual: 3 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
124-124: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
143-143: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
158-158: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
178-178: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
16-16: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
179-179: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
187-187: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
17-17: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
22-22: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
24-24: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
25-25: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
17-17: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
22-22: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
25-25: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
189-189: null (MD047, single-trailing-newline)
Files should end with a single newline character
Ruff
sleap_io/io/main.py
80-80: Use
X | Yfor type annotations (UP007)Convert to
X | Y
80-80: Use
X | Yfor type annotations (UP007)Convert to
X | Y
107-107: Use
X | Yfor type annotations (UP007)Convert to
X | Y
120-120: Use
X | Yfor type annotations (UP007)Convert to
X | Y
150-150: Use
X | Yfor type annotations (UP007)Convert to
X | Y
151-151: Use
X | Yfor type annotations (UP007)Convert to
X | Y
196-196: Use
X | Yfor type annotations (UP007)Convert to
X | Ysleap_io/model/video.py
9-9: Redefinition of unused
Optionalfrom line 9 (F811)Remove definition:
Optional
47-47: Use
X | Yfor type annotations (UP007)Convert to
X | Y
49-49: Use
X | Yfor type annotations (UP007)Convert to
X | Y
62-62: Use
X | Yfor type annotations (UP007)Convert to
X | Y
63-63: Use
X | Yfor type annotations (UP007)Convert to
X | Y
65-65: Use
X | Yfor type annotations (UP007)Convert to
X | Y
102-102: Use
tupleinstead ofTuplefor type annotation (UP006)Replace with
tuple
110-110: Use
tupleinstead ofTuplefor type annotation (UP006)Replace with
tuple
118-118: Do not use bare
except(E722)
189-192: Use
return all(Path(f).exists() for f in self.filename)instead offorloop (SIM110)Replace with
return all(Path(f).exists() for f in self.filename)
204-204: Use
X | Yfor type annotations (UP007)Convert to
X | Y
205-205: Use
X | Yfor type annotations (UP007)Convert to
X | Ysleap_io/io/labelstudio.py
36-36: Unnecessary open mode parameters (UP015)
Remove open mode parameters
39-39: Do not compare types, use
isinstance()(E721)
244-244: No explicit
stacklevelkeyword argument found (B028)
259-259: Loop control variable
indvnot used within loop body (B007)Rename unused
indvto_indvtests/io/test_slp.py
6-6:
sleap_io.Edgeimported but unused (F401)Remove unused import
9-9:
sleap_io.LabeledFrameimported but unused (F401)Remove unused import
10-10:
sleap_io.Trackimported but unused (F401)Remove unused import
11-11:
sleap_io.Pointimported but unused (F401)Remove unused import
12-12:
sleap_io.PredictedPointimported but unused (F401)Remove unused import
25-25:
sleap_io.io.slp.serialize_skeletonsimported but unused (F401)Remove unused import
29-29: Redefinition of unused
read_instancesfrom line 22 (F811)Remove definition:
read_instances
29-29:
sleap_io.io.slp.read_instancesimported but unused (F401)Remove unused import
36-36:
sleap_io.io.utils.read_hdf5_datasetimported but unused (F401)Remove unused import:
sleap_io.io.utils.read_hdf5_dataset
38-38:
simplejsonimported but unused (F401)Remove unused import:
simplejson
60-60: Do not compare types, use
isinstance()(E721)
194-194: Do not compare types, use
isinstance()(E721)
326-329: Combine
ifbranches using logicaloroperator (SIM114)Combine
ifbranchessleap_io/io/nwb.py
54-54: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
265-265: Loop control variable
track_indexnot used within loop body (B007)Rename unused
track_indexto_track_indextests/model/test_labels.py
3-3:
numpy.testing.assert_equalimported but unused (F401)Remove unused import:
numpy.testing.assert_equal
14-14:
sleap_io.load_videoimported but unused (F401)Remove unused import:
sleap_io.load_video
192-192: Found useless expression. Either assign it to a variable or remove it. (B018)
200-200: Found useless expression. Either assign it to a variable or remove it. (B018)
207-207: Found useless expression. Either assign it to a variable or remove it. (B018)
215-215: Found useless expression. Either assign it to a variable or remove it. (B018)
sleap_io/io/jabs.py
71-71: Use
X | Yfor type annotations (UP007)Convert to
X | Y
86-86: Use
listinstead ofListfor type annotation (UP006)Replace with
list
141-141: Use
key not in dictinstead ofkey not in dict.keys()(SIM118)Remove
.keys()
155-155: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
193-193: Use
X | Yfor type annotations (UP007)Convert to
X | Y
229-229: Use
listinstead ofListfor type annotation (UP006)Replace with
list
291-291: No explicit
stacklevelkeyword argument found (B028)
306-306: No explicit
stacklevelkeyword argument found (B028)
343-343: No explicit
stacklevelkeyword argument found (B028)
537-537: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()sleap_io/model/labels.py
17-17:
sleap_io.Instanceimported but unused (F401)Remove unused import:
sleap_io.Instance
88-88: Do not compare types, use
isinstance()(E721)
92-92: Do not compare types, use
isinstance()(E721)
183-183: Use
X | Yfor type annotations (UP007)Convert to
X | Y
183-183: Use
X | Yfor type annotations (UP007)Convert to
X | Y
217-217: Do not compare types, use
isinstance()(E721)
238-242: Use ternary operator
n_tracks = n_preds if untracked else len(self.tracks)instead ofif-else-block (SIM108)
344-344: Use
X | Yfor type annotations (UP007)Convert to
X | Y
532-532: Do not compare types, use
isinstance()(E721)
549-549: Do not compare types, use
isinstance()(E721)
602-605: Use ternary operator
inds2 = np.array([0]) if n0 == 1 else np.setdiff1d(np.arange(n0), inds1)instead ofif-else-block (SIM108)Replace
if-else-block withinds2 = np.array([0]) if n0 == 1 else np.setdiff1d(np.arange(n0), inds1)sleap_io/io/video.py
16-19: Use
contextlib.suppress(ImportError)instead oftry-except-pass(SIM105)Replace with
contextlib.suppress(ImportError)
21-24: Use
contextlib.suppress(ImportError)instead oftry-except-pass(SIM105)Replace with
contextlib.suppress(ImportError)
22-22:
imageio_ffmpegimported but unused; consider usingimportlib.util.find_specto test for availability (F401)
26-29: Use
contextlib.suppress(ImportError)instead oftry-except-pass(SIM105)Replace with
contextlib.suppress(ImportError)
27-27:
avimported but unused; consider usingimportlib.util.find_specto test for availability (F401)
55-55: Use
X | Yfor type annotations (UP007)Convert to
X | Y
57-57: Use
X | Yfor type annotations (UP007)Convert to
X | Y
57-57: Use
tupleinstead ofTuplefor type annotation (UP006)Replace with
tuple
58-58: Use
X | Yfor type annotations (UP007)Convert to
X | Y
64-64: Use
X | Yfor type annotations (UP007)Convert to
X | Y
65-65: Use
X | Yfor type annotations (UP007)Convert to
X | Y
87-87: Do not compare types, use
isinstance()(E721)
90-90: Do not compare types, use
isinstance()(E721)
161-161: Use
tupleinstead ofTuplefor type annotation (UP006)Replace with
tuple
167-167: Use
tupleinstead ofTuplefor type annotation (UP006)Replace with
tuple
486-486: Use
X | Yfor type annotations (UP007)Convert to
X | Y
492-492: Use
X | Yfor type annotations (UP007)Convert to
X | Y
493-493: Use
X | Yfor type annotations (UP007)Convert to
X | Y
528-530: Use a single
ifstatement instead of nestedifstatements (SIM102)
559-559: Use
tupleinstead ofTuplefor type annotation (UP006)Replace with
tuple
577-580: Use ternary operator
frame_idx = list(self.frame_map.keys())[0] if self.frame_map else 0instead ofif-else-block (SIM108)Replace
if-else-block withframe_idx = list(self.frame_map.keys())[0] if self.frame_map else 0sleap_io/io/slp.py
33-36: Use
contextlib.suppress(ImportError)instead oftry-except-pass(SIM105)Replace with
contextlib.suppress(ImportError)
376-376: Avoid equality comparisons to
True; useif embed:for truth checks (E712)Replace with
embed
784-784: Use
X | Yfor type annotations (UP007)Convert to
X | Y
970-975: Use
instances[instance_id][5] = inst_to_id.get(id(from_predicted), -1)instead of anifblock (SIM401)
Additional comments not posted (10)
docs/index.md (3)
130-138: Highlight the addition of a new functionality for fixing video paths. Ensure to add documentation or examples on how theprefix_mapworks.
153-153: Highlight the new feature of saving labels with embedded images, which enhances data portability and usability.
168-168: Highlight the addition of a method for creating training, validation, and test splits, which is crucial for model training processes.sleap_io/io/main.py (2)
25-25: The documentation for theembedparameter has been updated to reflect its flexible nature, allowing for multiple embedding options.
164-164: Convert thefilenameto a string using.as_posix()method for consistency in handling file paths across different operating systems.sleap_io/model/video.py (1)
271-271: Ensure that thenew_filenameis converted to a posix path, which standardizes the file handling across different operating systems.sleap_io/io/labelstudio.py (1)
46-48: Properly added provenance information to labels, enhancing traceability.tests/io/test_slp.py (1)
Line range hint
286-324: Comprehensive tests for embedding functionality with various options.sleap_io/io/nwb.py (1)
143-143: Provenance information correctly added to labels, enhancing traceability.sleap_io/io/jabs.py (1)
172-174: Properly track provenance inread_labelsby adding filename to the labels' provenance information.This change aligns with the PR's goal of enhancing provenance tracking across different modules.
| <a href="#__consent">Change cookie settings</a> | ||
|
|
||
| nav: | ||
| - Overview: index.md | ||
| - Changelog: https://github.com/talmolab/sleap-io/releases | ||
| - Core API: | ||
| - Data model: model.md | ||
| - Data formats: formats.md | ||
| - Model: model.md |
There was a problem hiding this comment.
Correct the indentation of the Model and Formats items under the Core API section to align with the expected YAML structure.
- - Model: model.md
- - Formats: formats.md
+ - Model: model.md
+ - Formats: formats.mdCommittable 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.
| - Model: model.md | |
| - Model: model.md |
Tools
yamllint
[warning] 154-154: wrong indentation: expected 6 but found 4 (indentation)
| sio.save_file(labels, "predictions.nwb") | ||
| # Or: | ||
| # labels.save("predictions.nwb") | ||
| labels.save("predictions.nwb") |
There was a problem hiding this comment.
Update method call to match the new method signature as per the PR changes.
- labels.save("predictions.nwb")
+ labels.save("predictions.nwb", embed=None)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.
| labels.save("predictions.nwb") | |
| labels.save("predictions.nwb", embed=None) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
==========================================
+ Coverage 95.82% 96.04% +0.22%
==========================================
Files 17 17
Lines 1962 2022 +60
==========================================
+ Hits 1880 1942 +62
+ Misses 82 80 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (4)
tests/model/test_labels.py (2)
Line range hint
3-3: Remove unused import:numpy.testing.assert_equal
Line range hint
14-14: Remove unused import:sleap_io.load_videosleap_io/io/slp.py (2)
Line range hint
33-36: Consider usingcontextlib.suppressfor cleaner handling of optional imports.- try: - import cv2 - except ImportError: - pass + from contextlib import suppress + with suppress(ImportError): + import cv2
Line range hint
784-784: Use the|operator for union type annotations to enhance readability.- def read_instances( - labels_path: str, - skeletons: list[Skeleton], - tracks: list[Track], - points: list[Point], - pred_points: list[PredictedPoint], - format_id: float, - ) -> list[Union[Instance, PredictedInstance]]: + def read_instances( + labels_path: str, + skeletons: list[Skeleton], + tracks: list[Track], + points: list[Point], + pred_points: list[PredictedPoint], + format_id: float, + ) -> list[Instance | PredictedInstance]:
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap_io/io/slp.py (5 hunks)
- tests/model/test_labels.py (1 hunks)
Additional context used
Ruff
tests/model/test_labels.py
3-3:
numpy.testing.assert_equalimported but unused (F401)Remove unused import:
numpy.testing.assert_equal
14-14:
sleap_io.load_videoimported but unused (F401)Remove unused import:
sleap_io.load_video
192-192: Found useless expression. Either assign it to a variable or remove it. (B018)
200-200: Found useless expression. Either assign it to a variable or remove it. (B018)
207-207: Found useless expression. Either assign it to a variable or remove it. (B018)
215-215: Found useless expression. Either assign it to a variable or remove it. (B018)
sleap_io/io/slp.py
33-36: Use
contextlib.suppress(ImportError)instead oftry-except-pass(SIM105)Replace with
contextlib.suppress(ImportError)
376-376: Avoid equality comparisons to
True; useif embed:for truth checks (E712)Replace with
embed
784-784: Use
X | Yfor type annotations (UP007)Convert to
X | Y
Additional comments not posted (4)
tests/model/test_labels.py (3)
540-570: The previous comment about adding error handling for the sum of split ratios exceeding 1 still applies.
421-537: The tests for thesplitmethod are comprehensive and cover various scenarios including edge cases with rounding and integer inputs. However, consider adding assertions to verify the actual contents of the splits to ensure that the splitting logic is not only dividing the counts correctly but also assigning the right frames to each split.
572-589: The tests formake_training_splits_saveare good as they verify the integrity of the saved splits. However, adding checks for the contents of the saved files can further ensure that the data is saved correctly and not just the metadata.sleap_io/io/slp.py (1)
970-972: This change has already been reviewed and approved.
sleap_io/io/slp.py
Outdated
| """ | ||
| if embed == True: |
There was a problem hiding this comment.
Simplify the truth check for better readability and performance.
- if embed == True:
+ if embed: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 embed == True: | |
| if embed: |
Tools
Ruff
376-376: Avoid equality comparisons to
True; useif embed:for truth checks (E712)Replace with
embed
| def test_make_training_splits(slp_real_data, tmp_path): | ||
| labels = load_slp(slp_real_data) | ||
| assert len(labels.user_labeled_frames) == 5 | ||
|
|
||
| train, val = labels.make_training_splits(0.8) | ||
| assert len(train) == 4 | ||
| assert len(val) == 1 | ||
|
|
||
| train, val = labels.make_training_splits(3) | ||
| assert len(train) == 3 | ||
| assert len(val) == 2 | ||
|
|
||
| train, val = labels.make_training_splits(0.8, 0.2) | ||
| assert len(train) == 4 | ||
| assert len(val) == 1 | ||
|
|
||
| train, val, test = labels.make_training_splits(0.8, 0.1, 0.1) | ||
| assert len(train) == 4 | ||
| assert len(val) == 1 | ||
| assert len(test) == 1 | ||
|
|
||
| train, val, test = labels.make_training_splits(n_train=0.6, n_test=1) | ||
| assert len(train) == 3 | ||
| assert len(val) == 1 | ||
| assert len(test) == 1 | ||
|
|
||
| train, val, test = labels.make_training_splits(n_train=1, n_val=1, n_test=1) | ||
| assert len(train) == 1 | ||
| assert len(val) == 1 | ||
| assert len(test) == 1 | ||
|
|
There was a problem hiding this comment.
The test for make_training_splits method seems to lack error handling for cases where the sum of the split ratios exceeds 1. This should raise an appropriate error to prevent data inconsistencies.
def make_training_splits(
self,
n_train: int | float,
n_val: int | float | None = None,
n_test: int | float | None = None,
save_dir: str | Path | None = None,
seed: int | None = None,
) -> tuple[Labels, Labels] | tuple[Labels, Labels, Labels]:
total = (n_train if n_train < 1 else 0) + (n_val if n_val and n_val < 1 else 0) + (n_test if n_test and n_test < 1 else 0)
if total > 1:
raise ValueError("The sum of n_train, n_val, and n_test should not exceed 1.")
# rest of the method
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores