Test plant trait computation for DicotPipeline #95
Conversation
WalkthroughThis pull request standardizes function signatures by adding trailing commas to parameter lists in several modules. In addition, new pytest fixtures have been introduced for canola and soy traits CSV paths. The testing suites have been updated; one test now iterates over individual frame instances with more granular attribute assertions, and another test expands trait pipeline verification with new imports, CSV data processing, and comprehensive trait computations. Changes
Assessment against linked issues
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
=======================================
Coverage 77.44% 77.44%
=======================================
Files 13 13
Lines 1419 1419
=======================================
Hits 1099 1099
Misses 320 320 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/test_series.py (3)
273-273: Remove unused variableexpected_labeled_frame_pts
This variable is never used, leading to an unnecessary assignment.- expected_labeled_frame_pts = expected_labeled_frame.instances[0]🧰 Tools
🪛 Ruff (0.8.2)
273-273: Local variable
expected_labeled_frame_ptsis assigned to but never usedRemove assignment to unused variable
expected_labeled_frame_pts(F841)
284-314: Use a more Pythonic iteration method
Instead of looping overrange(len(...)), consider iterating directly over(crown_instance, expected_labeled_frame_instance)in a zip to improve readability and reduce indexing.- for i in range(len(expected_labeled_frame.instances)): - crown_instance = crown_lf.instances[i] - expected_labeled_frame_instance = expected_labeled_frame.instances[i] + for crown_instance, expected_labeled_frame_instance in zip(crown_lf.instances, expected_labeled_frame.instances): ...
341-371: Consolidate repeated instance comparison logic
The logic here closely mirrors that in the preceding test, leading to code duplication. Extracting a helper function to compare instances could reduce redundancy and improve maintainability.tests/test_trait_pipelines.py (3)
75-75: Remove unused import
Theget_summaryfunction is not referenced anywhere in this file, so this import is unnecessary.- from sleap_roots.summary import get_summary🧰 Tools
🪛 Ruff (0.8.2)
75-75:
sleap_roots.summary.get_summaryimported but unusedRemove unused import:
sleap_roots.summary.get_summary(F401)
393-393: Remove unused variableinteger_traits
This tuple is never referenced in the code.- integer_traits = ( - "lateral_count", - "scanline_first_ind", - "scanline_last_ind", - )🧰 Tools
🪛 Ruff (0.8.2)
393-393: Local variable
integer_traitsis assigned to but never usedRemove assignment to unused variable
integer_traits(F841)
406-406: Use the more idiomatic membership check
PEP 8 style recommendsif trait not in csv_traits_list:instead ofif not trait in csv_traits_list:.- if not trait in csv_traits_list: + if trait not in csv_traits_list:🧰 Tools
🪛 Ruff (0.8.2)
406-406: Test for membership should be
not inConvert to
not in(E713)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/data/canola_7do/919QDUH.batch_traits.csvis excluded by!**/*.csvtests/data/canola_7do/919QDUH.traits.csvis excluded by!**/*.csvtests/data/soy_6do/6PR6AA22JK.batch_traits.csvis excluded by!**/*.csvtests/data/soy_6do/6PR6AA22JK.traits.csvis excluded by!**/*.csv
📒 Files selected for processing (6)
sleap_roots/convhull.py(2 hunks)sleap_roots/networklength.py(1 hunks)sleap_roots/points.py(2 hunks)tests/fixtures/data.py(2 hunks)tests/test_series.py(3 hunks)tests/test_trait_pipelines.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/test_trait_pipelines.py (3)
sleap_roots/summary.py (1) (1)
get_summary(9:67)tests/fixtures/data.py (4) (4)
canola_traits_csv(29:31)canola_batch_traits_csv(35:37)soy_traits_csv(125:127)soy_batch_traits_csv(131:133)sleap_roots/trait_pipelines.py (4) (4)
compute_plant_traits(328:386)compute_batch_traits(635:677)summary_traits(249:255)csv_traits(258:269)
🪛 Ruff (0.8.2)
tests/test_series.py
273-273: Local variable expected_labeled_frame_pts is assigned to but never used
Remove assignment to unused variable expected_labeled_frame_pts
(F841)
tests/test_trait_pipelines.py
75-75: sleap_roots.summary.get_summary imported but unused
Remove unused import: sleap_roots.summary.get_summary
(F401)
393-393: Local variable integer_traits is assigned to but never used
Remove assignment to unused variable integer_traits
(F841)
406-406: Test for membership should be not in
Convert to not in
(E713)
🔇 Additional comments (12)
sleap_roots/points.py (2)
266-267: Adding trailing commas in function parameters is good practice.The addition of a trailing comma after the parameter type annotation follows good Python style practices. This makes future changes cleaner in version control diffs when parameters are added later.
285-286: Consistent style with trailing comma in parameter list.This change matches the trailing comma style used in the
get_left_normalized_vectorfunction, ensuring consistency throughout the codebase. This pattern makes future parameter additions cleaner in git diffs.sleap_roots/convhull.py (2)
609-610: Consistent trailing comma style in function signatures.The addition of a trailing comma in the parameter list is consistent with the style changes in other files. This improves code maintainability by making future parameter additions show cleaner diffs in version control.
625-626: Trailing comma addition follows consistent style pattern.This change maintains consistency with other function signature changes in the codebase. The trailing comma after function parameters is a good practice that improves readability and git diff clarity.
sleap_roots/networklength.py (1)
38-39: Trailing comma addition is consistent with other function signature updates.The addition of a trailing comma after the parameter type annotation follows the same pattern applied to other functions in the codebase. This improves maintainability by making future parameter additions cleaner in version control diffs.
tests/fixtures/data.py (4)
28-32: Good addition of fixture for canola traits CSV path.This fixture provides a path to computed traits CSV for 7-day-old canola, which supports the PR objective of testing plant trait computation for the DicotPipeline. The fixture follows consistent naming and documentation patterns.
34-38: Complementary fixture for canola batch traits CSV.This fixture pairs well with the individual traits CSV fixture, providing the batch-level traits path. This will allow more comprehensive testing of the DicotPipeline with both individual and batch-level trait calculations.
124-128: Useful addition of soy traits CSV fixture.This fixture provides test data path for soy plants, which expands testing coverage to another dicot species. Having both canola and soy test data will improve the robustness of the DicotPipeline tests.
130-134: Complete test data coverage with soy batch traits.This addition completes the set of fixtures needed for comprehensive testing of the DicotPipeline across multiple dicot species (canola and soy) and at both individual and batch levels.
tests/test_trait_pipelines.py (3)
20-74: Imports appear consistent
These newly introduced imports align well with the expanded trait calculations below. No immediate issues noted.
202-465: Extensive trait-by-frame validation
This block provides thorough testing of per-frame computations for canola and soy, correctly verifying shape, type, and range checks. No issues found.🧰 Tools
🪛 Ruff (0.8.2)
393-393: Local variable
integer_traitsis assigned to but never usedRemove assignment to unused variable
integer_traits(F841)
406-406: Test for membership should be
not inConvert to
not in(E713)
466-573: Comprehensive batch trait aggregation
Verifying aggregated statistics and ensuring final data integrity is well-handled here. It meaningfully complements the per-frame checks.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_trait_pipelines.py (1)
399-403:⚠️ Potential issueFix the string concatenation in angle_traits.
The code inadvertently merges"primary_angle_distal"and"lateral_angles_distal"into a single string. This is a repeat of a past comment.angle_traits = ( "primary_angle_proximal", - "primary_angle_distal" "lateral_angles_distal", + "primary_angle_distal", + "lateral_angles_distal", "lateral_angles_proximal", )
🧹 Nitpick comments (5)
tests/test_trait_pipelines.py (5)
75-75: Remove the unused import.
get_summaryis never called in this file, triggering a warning (F401). Consider removing it:- from sleap_roots.summary import get_summary🧰 Tools
🪛 Ruff (0.8.2)
75-75:
sleap_roots.summary.get_summaryimported but unusedRemove unused import:
sleap_roots.summary.get_summary(F401)
148-150: Variable overshadowing can reduce clarity.
Since each parameter is reused as a local variable name for the DataFrame, consider a minor rename to differentiate them:- canola_traits_csv = pd.read_csv(canola_traits_csv) + canola_traits_df = pd.read_csv(canola_traits_csv)
152-154: Same overshadowing concern for soy variables.
For clarity, consider the same rename pattern:- soy_traits_csv = pd.read_csv(soy_traits_csv) + soy_traits_df = pd.read_csv(soy_traits_csv)
394-394: Remove the unusedinteger_traitsassignment.
It’s never consumed, so it triggers a warning (F841). Removing it will tidy the code:- integer_traits = ( - "lateral_count", - "scanline_first_ind", - "scanline_last_ind", - )🧰 Tools
🪛 Ruff (0.8.2)
394-394: Local variable
integer_traitsis assigned to but never usedRemove assignment to unused variable
integer_traits(F841)
407-407: Use Python’s recommended membership style.
Replace “if not trait in ...” with “if trait not in ...” to align with idiomatic Python:- if not trait in csv_traits_list: + if trait not in csv_traits_list:🧰 Tools
🪛 Ruff (0.8.2)
407-407: Test for membership should be
not inConvert to
not in(E713)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_trait_pipelines.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/test_trait_pipelines.py (6)
sleap_roots/lengths.py (2) (2)
get_max_length_pts(8:53)get_root_lengths(56:86)sleap_roots/bases.py (1) (1)
get_root_widths(224:427)sleap_roots/tips.py (3) (3)
get_tips(6:29)get_tip_xs(32:50)get_tip_ys(53:78)sleap_roots/summary.py (1) (1)
get_summary(9:67)tests/fixtures/data.py (4) (4)
canola_traits_csv(29:31)canola_batch_traits_csv(35:37)soy_traits_csv(125:127)soy_batch_traits_csv(131:133)sleap_roots/trait_pipelines.py (4) (4)
compute_plant_traits(328:386)compute_batch_traits(635:677)summary_traits(249:255)csv_traits(258:269)
🪛 Ruff (0.8.2)
tests/test_trait_pipelines.py
75-75: sleap_roots.summary.get_summary imported but unused
Remove unused import: sleap_roots.summary.get_summary
(F401)
394-394: Local variable integer_traits is assigned to but never used
Remove assignment to unused variable integer_traits
(F841)
407-407: Test for membership should be not in
Convert to not in
(E713)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests (windows-2022, Python 3.11)
🔇 Additional comments (14)
tests/test_trait_pipelines.py (14)
20-22: All imports are used correctly.
No issues found regarding these import statements.
24-34: All newly imported base-related utilities appear to be correctly utilized.
These functions are indeed called in the subsequent logic. No concerns here.
36-40: Tip-related imports seem appropriate.
They are referenced correctly when creating or extracting tip points.
42-46: Scanline-related imports look good.
They are used for intersection counts and indices without apparent issues.
48-48: Angle and node index functions are properly imported and used.
No concerns regarding correctness or usage.
50-57: Network length and solidity imports are valid.
They are correctly consumed for computing network-based traits.
59-66: Convex hull imports are valid and appear consistent with usage.
No issues detected.
68-73: Ellipse-related imports are used accurately.
The code fits ellipses and extracts ellipse parameters as intended.
126-129: New fixture parameters for dicot traits CSVs are properly integrated.
All references to these fixtures are consistent with the subsequent code.
156-157: Computed batch traits and their concatenation look fine.
No issues with data concatenation or shape.
159-161: Shape checks for canola traits appear solid.
No detected issues.
163-167: Shape checks for soy traits and batch traits look aligned.
No concerns regarding these assertions.
169-181: Frame equality checks are appropriate.
Asserting equality with tolerances promotes numerical stability. Good practice.
183-188: Angle traits tuple is clearly defined.
No functional issues here.
| @@ -67,7 +123,12 @@ def test_dicot_pipeline( | |||
| canola_lateral_slp, | |||
| soy_primary_slp, | |||
| soy_lateral_slp, | |||
| canola_traits_csv, | |||
There was a problem hiding this comment.
add a docstring explaining this test. particularly mentioning that it compares the computed traits using DicotPipeline with a fixture, with the manually computed traits from function chaining
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/test_trait_pipelines.py (3)
124-139: Add parameter documentation.Parameters like
canola_traits_csvandsoy_traits_csvaren't described in the docstring. Including a short explanation or referencing their usage (e.g., “Path to computed traits CSV”) would improve clarity.
157-177: Beware of hard-coded shape checks.The tests assert
72, 117for canola/soy shapes. If the underlying dataset changes or if more frames/columns are added, these checks will break. Consider either explaining why these shapes are fixed or make the size checks a bit more flexible if the data is expected to scale.
215-503: Confirm angle std usage.Angle-based traits are range-limited to [0, 180], but their
_stdsummary can legitimately exceed 90 depending on data distribution. Currently,_stdis excluded from the bounded range checks, which is intentional. Just make sure this aligns with your domain assumptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_series.py(2 hunks)tests/test_trait_pipelines.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/test_trait_pipelines.py (11)
sleap_roots/lengths.py (3) (3)
get_max_length_pts(8-53)get_root_lengths(56-86)get_curve_index(89-136)sleap_roots/points.py (3) (3)
get_count(53-64)join_pts(67-99)get_all_pts_array(102-139)sleap_roots/bases.py (9) (9)
get_root_widths(224-427)get_base_xs(72-90)get_base_ys(93-112)get_bases(10-31)get_base_ct_density(131-166)get_base_length(115-128)get_base_length_ratio(169-190)get_base_median_ratio(193-221)get_base_tip_dist(34-69)sleap_roots/tips.py (3) (3)
get_tips(6-29)get_tip_xs(32-50)get_tip_ys(53-78)sleap_roots/scanline.py (3) (3)
count_scanline_intersections(7-57)get_scanline_first_ind(60-75)get_scanline_last_ind(78-93)sleap_roots/angle.py (1) (1)
get_root_angle(52-93)sleap_roots/networklength.py (6) (6)
get_network_solidity(101-118)get_network_length(62-98)get_network_distribution(121-178)get_network_width_depth_ratio(38-59)get_network_distribution_ratio(181-205)get_bbox(9-35)sleap_roots/convhull.py (6) (6)
get_convhull(15-48)get_chull_area(81-112)get_chull_perimeter(51-78)get_chull_max_height(149-179)get_chull_max_width(115-146)get_chull_line_lengths(182-205)tests/fixtures/data.py (4) (4)
canola_traits_csv(29-31)canola_batch_traits_csv(35-37)soy_traits_csv(125-127)soy_batch_traits_csv(131-133)sleap_roots/trait_pipelines.py (4) (4)
compute_plant_traits(328-386)compute_batch_traits(635-677)summary_traits(249-255)csv_traits(258-269)sleap_roots/series.py (2) (2)
get_primary_points(314-338)get_lateral_points(340-364)
🔇 Additional comments (4)
tests/test_trait_pipelines.py (4)
20-73: Imports look good.These additions appear consistent with functions used below. No immediate concerns regarding unused imports or potential naming conflicts.
192-213: Constants and typed sets are well-organized.Defining
angle_traits,numeric_types,summary_suffixes, and thedicotsdictionary up front promotes readability. The approach is sound, and there's no immediate issue here.
215-503: Validate array shapes in trait computations.Within this large loop, many calls to
get_ 8000 *functions assume consistent array shapes (e.g.,get_primary_pointsalways returns an array with expected shape). If any method returns inconsistent shapes or empty arrays, future indexing might fail. Consider adding tests or fallback checks for shape mismatches, ensuring graceful handling (e.g., skip assertions when arrays are empty or shapes mismatch).
505-581: Check for negative coordinate-based traits.All non-angle traits are assumed nonnegative. If bounding box coordinates include negative values (e.g., an image origin offset), certain derived traits (like
chull_max_widthorchull_max_height) might remain valid but negative in theory. Ensure this is correct given the data domain.
sleap-roots/tests/test_trait_pipelines.py
Line 63 in 3f0861d
Fixes #85
Summary by CodeRabbit
test_dicot_pipelinefunction to compute traits separately for canola and soy, with expanded assertions for trait shape validation.