8000 Refactor angle calculation in DicotPipeline by andrewjoc · Pull Request #100 · talmolab/sleap-roots · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Refactor angle calculation in DicotPipeline #100

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

Closed
wants to merge 21 commits into from

Conversation

andrewjoc
Copy link
Contributor
@andrewjoc andrewjoc commented Mar 23, 2025
  • changed calculation of angle traits in the DicotPipeline to be more modular
  • replaced get_root_angle with get_root_vectors and get_vector_angles_from_gravity in DicotPipline by adding new trait definitions
  • updated DicotPipeline tests in test_trait_pipelines.py
  • added notebook DicotRootAngle.ipynb explaining new method for calculating root angles

Fixes #97

Summary by CodeRabbit

  • New Features

    • Enhanced vector calculations to clearly flag near-zero cases, improving accuracy in edge scenarios.
    • Improved coordinate extraction by accepting both single and multiple index inputs with robust error handling.
    • Updated trait computations to include additional vector-based definitions for lateral and primary outputs, with some new values excluded from CSV exports.
  • Tests

    • Expanded test coverage to ensure correct behavior for the updated calculations and input validations, including new tests for root vector computations.
    • Introduced new fixtures to support testing scenarios with NaN values and 2D points.

Copy link
Contributor
coderabbitai bot commented Mar 23, 2025

Walkthrough

The pull request introduces modifications across multiple modules to enhance vector and node handling. In sleap_roots/angle.py, the function get_node_ind now specifies its return type and handles NaN values more effectively. The get_vector_angles_from_gravity function has been updated to manage zero-length vectors. In sleap_roots/points.py, the get_nodes function accepts both integers and arrays with additional error checks, while get_root_vectors has improved validation. The DicotPipeline in sleap_roots/trait_pipelines.py has updated tr 8000 ait definitions to reflect these changes. Tests in tests/test_points.py and tests/test_trait_pipelines.py have been expanded accordingly.

Changes

Files Change Summary
sleap_roots/angle.py Updated get_node_ind return type and NaN handling; modified get_vector_angles_from_gravity for zero-length vectors.
sleap_roots/points.py Enhanced get_nodes to accept multiple index types with error handling; improved validation in get_root_vectors.
sleap_roots/trait_pipelines.py Updated trait definitions in DicotPipeline to replace deprecated functions with new modular ones and added new methods for root vectors.
tests/test_points.py, tests/test_trait_pipelines.py Added tests for get_root_vectors functionality and updated trait pipeline tests; introduced fixtures for sample data.

Assessment against linked issues

Objective Addressed Explanation
Refactor angle function for DicotPipeline to use modular vector functions (get_vector_angles_from_gravity & get_root_vectors) (#97)

Possibly related PRs

  • Test plant trait computation for DicotPipeline #95: The changes in the main PR are related to the modifications in the get_node_ind function, which is also referenced in the retrieved PR's updates to the test_dicot_pipeline function, as it now includes this function in its calculations.

Suggested reviewers

  • eberrigan

Poem

I'm a rabbit, hopping with delight,
Code sprouts fresh in the morning light.
Vectors and nodes now dance in tune,
Near-zero cases handled before noon.
Trait pipelines sing a brand new song,
In our code garden, all is strong.
Happy hops to progress all day long!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4516ef8 and 83feaa8.

📒 Files selected for processing (1)
  • sleap_roots/points.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • sleap_roots/points.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Tests (windows-2022, Python 3.11)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
codecov bot commented Mar 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.33%. Comparing base (8958e82) to head (4516ef8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
+ Coverage   77.44%   78.33%   +0.88%     
==========================================
  Files          13       13              
  Lines        1419     1431      +12     
==========================================
+ Hits         1099     1121      +22     
+ Misses        320      310      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@andrewjoc andrewjoc marked this pull request as ready for review March 24, 2025 17:08
Copy link
Contributor
@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
sleap_roots/points.py (1)

222-245: 🛠️ Refactor suggestion

Vector definition conflicts with function description.

Per the docstring, “the vector from start to end” implies end_nodes - start_nodes, but the code does start_nodes - end_nodes. The resulting vectors may be reversed compared to the function’s stated intent. Consider reversing the subtraction, or clarifying the docstring:

-    vectors = start_nodes - end_nodes
+    vectors = end_nodes - start_nodes
🧹 Nitpick comments (2)
tests/test_trait_pipelines.py (1)

54-58: Remove unused import to maintain cleanliness.

get_root_angle is not referenced in this file and raises a warning from static analysis. Consider removing it:

-from sleap_roots.angle import (
-    get_root_angle,
-    get_node_ind,
-    get_vector_angles_from_gravity,
-)
+from sleap_roots.angle import (
+    get_node_ind,
+    get_vector_angles_from_gravity,
+)
🧰 Tools
🪛 Ruff (0.8.2)

55-55: sleap_roots.angle.get_root_angle imported but unused

Remove unused import: sleap_roots.angle.get_root_angle

(F401)

sleap_roots/points.py (1)

142-159: Typo in docstring and potential mismatch in error name.

There's a spelling error in the docstring (TypeEror) that should be TypeError. Consider correcting it for clarity and consistency:

-        TypeEror: If node_index is not an integer or array of integers.
+        TypeError: If node_index is not an integer or array of integers.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8958e82 and 8393bb9.

📒 Files selected for processing (5)
  • sleap_roots/angle.py (2 hunks)
  • sleap_roots/points.py (3 hunks)
  • sleap_roots/trait_pipelines.py (4 hunks)
  • tests/test_points.py (2 hunks)
  • tests/test_trait_pipelines.py (3 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
tests/test_trait_pipelines.py (2)
sleap_roots/points.py (4)
  • get_count (53-64)
  • join_pts (67-99)
  • get_nodes (142-206)
  • get_root_vectors (209-244)
sleap_roots/angle.py (2)
  • get_root_angle (52-93)
  • get_vector_angles_from_gravity (96-127)
tests/test_points.py (2)
sleap_roots/points.py (2)
  • get_root_vectors (209-244)
  • get_nodes (142-206)
tests/test_angle.py (1)
  • pts_nan32_5node (100-118)
sleap_roots/trait_pipelines.py (2)
sleap_roots/points.py (2)
  • get_root_vectors (209-244)
  • get_nodes (142-206)
sleap_roots/angle.py (1)
  • get_vector_angles_from_gravity (96-127)
🪛 Ruff (0.8.2)
tests/test_trait_pipelines.py

55-55: sleap_roots.angle.get_root_angle imported but unused

Remove unused import: sleap_roots.angle.get_root_angle

(F401)

🔇 Additional comments (25)
sleap_roots/angle.py (2)

108-111: Great addition for handling near-zero vectors.

Marking vectors that are effectively zero within a small tolerance allows the results to gracefully reflect this edge case.


121-123: Correct approach for ignoring invalid angles.

Assigning np.nan to angles associated with near-zero vectors ensures upstream calculations do not treat them as meaningful data.

tests/test_trait_pipelines.py (2)

22-28: Imports align with new trait logic.

These imports for get_nodes and get_root_vectors are consistent with the updated pipeline approach.


345-382: New node-based angle calculations look consistent.

This shift from directly computing angles per node to computing vectors via get_root_vectors and then applying get_vector_angles_from_gravity is modular and clear. The usage of these functions matches their intended signatures.

sleap_roots/points.py (2)

160-184: Single-instance logic for float node indices is unconventional.

Allowing node_index == 0.0 as a valid index while raising an error for other float values is unusual and might introduce confusion or hidden bugs. Consider always enforcing an integer type or existing array-of-int approach to avoid ambiguous float usage.


187-201: Multiple-instance shape checks are robust.

The checks ensure the length of node_index matches the number of instances and that it consists of integers. This helps prevent misalignment between node arrays and instances.

tests/test_points.py (6)

26-26: Import of get_root_vectors is appropriately added.

The import is correctly added to the existing list of imports from sleap_roots.points.


864-896: Well-structured tests for valid get_root_vectors functionality.

The test function provides comprehensive coverage for various input scenarios including:

  • Basic vector inputs with different shapes
  • Mixed dimension inputs
  • Handling of NaN values
  • Verification of correct vector calculations

The use of allclose with equal_nan=True is appropriate for comparing results containing NaN values.


898-927: Good error handling verification for get_root_vectors.

The test checks that appropriate ValueError exceptions are raised for all invalid input scenarios:

  • Empty inputs
  • Coordinates with incorrect dimensions
  • Inputs with different number of instances
  • Arrays with excessive dimensions

This ensures robustness of the function when handling potential misuse.


930-949: Well-defined test fixtures providing reusable test data.

These fixtures provide well-structured test data with deliberate NaN values that can be reused across multiple tests. The pts_nan32_5node fixture creates a multi-instance dataset with specific NaN values at known positions, while pts_2d provides a simpler single-instance dataset.


951-1000: Comprehensive get_nodes tests with valid inputs.

The test covers important validation scenarios:

  • Single instance, integer node index
  • Support for float 0.0 as an index
  • Array index support
  • Multiple instances with integer and array indices

Good use of allclose with equal_nan=True parameter to properly compare arrays containing NaN values.


1002-1042: Thorough negative testing for get_nodes.

Tests cover all important error cases:

  • Out of bounds indices
  • Invalid index types
  • Array indices that don't match instance count

This ensures the function properly validates its inputs before attempting to process them.

sleap_roots/trait_pipelines.py (13)

68-68: Import of get_root_vectors is correctly added.

The function is appropriately imported from sleap_roots.points module to support the refactored angle calculation logic.


930-937: Improved modularity with lateral_distal_node_pts.

The trait definition now uses the more generic get_nodes function instead of hard-coding node selection, allowing for more flexibility in node selection.


939-946: Enhanced vector calculation with new lateral_distal_root_vectors trait.

This new intermediate trait extracts the vectors between distal nodes and base points, improving modularity by separating vector calculation from angle calculation.


948-955: Refactored angle calculation for lateral_angles_distal.

The trait now uses get_vector_angles_from_gravity instead of get_root_angle, leveraging the precomputed vectors. This approach:

  1. Improves code structure by separating concerns
  2. Handles near-zero vectors more consistently (marks them as NaN)
  3. Aligns with vector-based computation best practices

957-964: Added lateral_proximal_node_pts for consistent handling of proximal nodes.

Similar to the distal node trait, this provides consistent node extraction using get_nodes, improving the pipeline modularity.


966-973: Added lateral_proximal_root_vectors for vector-based angle calculation.

This intermediate trait enables the vector-based approach for proximal angle calculation, providing consistency with the distal angle implementation.


975-981: Refactored lateral_angles_proximal for vector-based calculation.

The trait now uses the same approach as lateral_angles_distal, ensuring consistency in how angles are calculated throughout the pipeline.


1033-1040: Added primary_proximal_node_pt to extract specific primary nodes.

This trait definition enables extracting specific primary root nodes in a more reusable way, consistent with the approach used for lateral roots.


1042-1049: Added primary_proximal_root_vector for vector-based primary angle calculation.

This trait computes vectors for primary roots using the same pattern established for lateral roots, promoting code consistency.


1051-1057: Refactored primary_angle_proximal to use vector-based angle calculation.

The primary root angle calculation now uses the same vector-based approach as lateral roots, ensuring consistency across the codebase.


1070-1077: Added primary_distal_node_pt for consistent node extraction.

This trait follows the same pattern as the other node extraction traits, maintaining a consistent approach throughout the pipeline.


1079-1086: Added primary_distal_root_vector for distal vector calculation.

This trait completes the set of vector calculations needed for the refactored angle calculations.


1088-1094: Refactored primary_angle_distal to use vector-based calculation.

The final angle calculation trait now uses the vector-based approach, completing the consistent refactoring of all angle calculations in the pipeline.

@andrewjoc andrewjoc requested a review from eberrigan March 24, 2025 18:14
Copy link
Collaborator
@eberrigan eberrigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's try to simplify the checks and make sure inputs past to get_root_vectors are in the right order whenever used

@@ -190,14 +219,26 @@ def get_root_vectors(start_nodes: np.ndarray, end_nodes: np.ndarray) -> np.ndarr
An array of vectors with shape (instances, 2), representing the vector from start
to end for each instance.
"""
# If start or end nodes is not a 2D array, make it one
start_nodes = np.atleast_2d(start_nodes)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after this shouldn't dimension of arrays both be 2? can we use that to simplify the logic?

Copy link
Contributor
@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
sleap_roots/points.py (2)

232-232: Unnecessary f-string.

The f-string doesn't contain any placeholders, making the f prefix unnecessary.

-    raise ValueError(f"start_nodes and end_nodes do not have the same shape.")
+    raise ValueError("start_nodes and end_nodes do not have the same shape.")
🧰 Tools
🪛 Ruff (0.8.2)

232-232: f-string without any placeholders

Remove extraneous f prefix

(F541)


198-198: Style improvement for condition check.

Consider rewriting the Yoda condition for better readability.

-        if not np.all((0 <= node_index) & (node_index < pts.shape[1])):
+        if not np.all((node_index >= 0) & (node_index < pts.shape[1])):
🧰 Tools
🪛 Ruff (0.8.2)

198-198: Yoda condition detected

Rewrite as node_index >= 0

(SIM300)

sleap_roots/trait_pipelines.py (2)

1085-1085: Minor description inconsistency.

The description states "Array of primary distal points" but the trait is for root vectors. This should be more specific to clarify it's for vectors.

-                description="Array of primary distal points `(instances, 2)`.",
+                description="Array of primary distal root vectors `(instances, 2)`.",

930-1094: Overall angle calculation refactoring is well implemented.

The PR has successfully replaced the direct get_root_angle function with a more modular two-step approach using get_root_vectors and get_vector_angles_from_gravity. This improves modularity and clarity in the code by:

  1. First extracting node points
  2. Then computing vectors between points
  3. Finally calculating angles from these vectors

This change aligns perfectly with the PR objective to enhance modularity of angle trait calculations.

The new approach also provides better separation of concerns and makes the code more maintainable. If you need to modify vector calculations or angle calculations in the future, you can do so independently.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4d3c596 and 90c1012.

📒 Files selected for processing (5)
  • sleap_roots/angle.py (5 hunks)
  • sleap_roots/points.py (3 hunks)
  • sleap_roots/trait_pipelines.py (4 hunks)
  • tests/test_points.py (2 hunks)
  • tests/test_trait_pipelines.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • sleap_roots/angle.py
🧰 Additional context used
🧬 Code Definitions (3)
tests/test_trait_pipelines.py (2)
sleap_roots/points.py (2)
  • get_nodes (142-205)
  • get_root_vectors (208-236)
sleap_roots/angle.py (1)
  • get_vector_angles_from_gravity (96-128)
tests/test_points.py (2)
sleap_roots/points.py (2)
  • get_root_vectors (208-236)
  • get_nodes (142-205)
tests/test_angle.py (1)
  • pts_nan32_5node (100-118)
sleap_roots/trait_pipelines.py (2)
sleap_roots/points.py (2)
  • get_root_vectors (208-236)
  • get_nodes (142-205)
sleap_roots/angle.py (1)
  • get_vector_angles_from_gravity (96-128)
🪛 Ruff (0.8.2)
sleap_roots/points.py

198-198: Yoda condition detected

Rewrite as node_index >= 0

(SIM300)


232-232: f-string without any placeholders

Remove extraneous f prefix

(F541)

🪛 GitHub Check: codecov/patch
sleap_roots/points.py

[warning] 180-180: sleap_roots/points.py#L180
Added line #L180 was not covered by tests


[warning] 203-203: sleap_roots/points.py#L203
Added line #L203 was not covered by tests

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Tests (windows-2022, Python 3.11)
🔇 Additional comments (26)
tests/test_trait_pipelines.py (3)

22-28: Clear import organization for new vector-based calculation.

The addition of imports from sleap_roots.points including get_nodes and get_root_vectors appropriately supports the refactoring of angle calculations to use a vector-based approach.


54-57: Good modularization of angle calculations.

Replacing the import of get_root_angle with get_vector_angles_from_gravity aligns well with the PR objectives to enhance modularity of angle trait calculations.


344-381: Well-structured implementation of vector-based angle calculation.

The implementation follows a clear logical flow:

  1. Extract node points using get_nodes
  2. Calculate vectors between points using get_root_vectors
  3. Compute angles using get_vector_angles_from_gravity

This approach is more modular and reusable than the previous implementation, properly separating coordinate extraction, vector creation, and angle calculation into distinct steps.

tests/test_points.py (4)

864-891: Comprehensive tests for get_root_vectors with valid inputs.

The test cases cover various scenarios:

  • Basic vector calculation with different input shapes
  • Handling of NaN values
  • Validation of output shapes

This thorough testing approach helps ensure the function behaves correctly across different input types and edge cases.


893-905: Good error handling validation for get_root_vectors.

The tests properly validate that the function raises appropriate exceptions for:

  • Mismatched input shapes
  • Inputs with too many dimensions

These tests ensure robust error handling in the implementation.


908-940: Useful test fixtures for reusable test data.

These fixtures provide well-structured test data that can be reused across multiple tests, including edge cases with NaN values. This promotes test code reusability and consistency.


943-997: Thorough testing of get_nodes function with multiple scenarios.

The tests cover:

  • Different input shapes (single/multiple instances)
  • Integer and array index handling
  • Error cases for out-of-bounds indices
  • Correct output shape validation

This comprehensive testing ensures the enhanced functionality works reliably.

sleap_roots/points.py (5)

142-160: Improved function signature and documentation for enhanced flexibility.

The updated signature now supports both integer and array indices, with clear documentation of the parameter types and return values. This makes the function more versatile while maintaining clarity about its behavior.


167-189: Clean implementation for handling single instance inputs.

The approach of tracking expanded dimensions and conditionally squeezing the output ensures consistent return shapes based on input types. This maintains backward compatibility while adding new functionality.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 180-180: sleap_roots/points.py#L180
Added line #L180 was not covered by tests


191-205: Robust validation for array indices.

The implementation includes thorough checks for:

  • Array length matching number of instances
  • Indices being within bounds
  • Array containing only integer types

This comprehensive validation helps prevent runtime errors and provides clear error messages.

🧰 Tools
🪛 Ruff (0.8.2)

198-198: Yoda condition detected

Rewrite as node_index >= 0

(SIM300)

🪛 GitHub Check: codecov/patch

[warning] 203-203: sleap_roots/points.py#L203
Added line #L203 was not covered by tests


222-228: Improved input handling for vector calculation.

Using np.atleast_2d ensures inputs are properly shaped, and the dimension check prevents errors with higher-dimensional inputs. This makes the function more robust against unexpected input types.


235-236: Vector calculation corrected for proper directionality.

The vector calculation now properly subtracts start points from end points, which is the mathematically correct way to calculate vectors pointing from start to end.

sleap_roots/trait_pipelines.py (14)

68-68: Import added for the new vector computation.

The addition of get_root_vectors import is consistent with the PR objective to enhance angle calculations modularity.


930-937: New trait for lateral distal node points enhances modularity.

This new trait definition extracts lateral distal node points as the first step in the refactored angle calculation process. This is a good separation of concerns that increases modularity.


938-946: New vector computation trait adds clarity.

This new lateral_distal_root_vectors trait creates vectors from base points to distal nodes, which makes the angle calculation process more clear and modular. Setting include_in_csv=False is appropriate since this is an intermediate computation.


947-955: Updated angle calculation uses the new vector approach.

The lateral_angles_distal trait now uses the vector-based approach with get_vector_angles_from_gravity instead of the previous get_root_angle function. This is consistent with the PR objective of refactoring angle calculations.


957-964: New trait for lateral proximal node points follows the same pattern.

This follows the same modular approach as used for distal node points, maintaining consistency in the implementation.


965-973: New proximal vector computation trait.

The trait definition correctly uses the base and proximal node points to compute vectors, consistent with the distal implementation.


974-981: Lateral proximal angle calculation updated to use vectors.

The implementation consistently uses the new vector-based approach for angle calculation.


1033-1040: Primary proximal node point extraction trait.

This trait follows the same pattern as the lateral traits, maintaining a consistent approach to node extraction throughout the codebase.


1041-1049: Primary proximal root vector computation trait.

The trait definition for computing the primary proximal root vector is correctly implemented and follows the established pattern.


1050-1056: Updated primary proximal angle calculation.

The primary angle calculation now correctly uses the vector-based approach with get_vector_angles_from_gravity.


1070-1077: Primary distal node point extraction trait.

This implements the node extraction for primary distal points, consistent with the implementation approach for the other node points.


1078-1086: Primary distal root vector computation trait.

The implementation is correct and consistent with the other vector computation traits.


1087-1094: Updated primary distal angle calculation.

The primary distal angle calculation now correctly uses the vector-based approach, completing the refactoring of all angle calculations to use the new method.


938-946: Verify the order of input traits in vector computation.

Previous reviewers have mentioned to "check order of input traits." For the vector computation traits, verify that start_nodes and end_nodes are in the correct order in all instances:

  • lateral_base_pts should be the start nodes
  • The node points (lateral_distal_node_pts, etc.) should be the end nodes

This matches the expectation of get_root_vectors which calculates from start to end.

Also applies to: 965-973, 1041-1049, 1078-1086

Copy link
Contributor
@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
sleap_roots/trait_pipelines.py (4)

930-937: Clarify the resulting shape in docstring.

While this trait definition cites (instances, nodes, 2), if lateral_distal_node_inds is a single integer per instance, the actual shape might be (instances, 2). Consider updating the docstring to reflect A36C both possibilities.


957-964: Docstring shape detail.

Similar to the distal node points docstring, “(instances, nodes, 2)” might appear as “(instances, 2)” if only one node is used. Slight docstring clarification would help future maintainers.


1033-1040: Potential docstring shape mismatch.

The docstring says (instances, nodes, 2); call to get_nodes with a single integer node index often yields (instances, 2). Consider reflecting both possibilities.


1070-1077: Docstring adaptation recommended.

Similar to above, specifying (instances, nodes, 2) might be misleading if only one node is retrieved. Updating the docstring can reduce confusion.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 90c1012 and 9a12e53.

📒 Files selected for processing (2)
  • sleap_roots/points.py (3 hunks)
  • sleap_roots/trait_pipelines.py (4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
sleap_roots/trait_pipelines.py (2)
sleap_roots/points.py (2)
  • get_root_vectors (212-239)
  • get_nodes (142-209)
sleap_roots/angle.py (1)
  • get_vector_angles_from_gravity (96-128)
🪛 GitHub Check: codecov/patch
sleap_roots/points.py

[warning] 174-174: sleap_roots/points.py#L174
Added line #L174 was not covered by tests

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Tests (windows-2022, Python 3.11)
🔇 Additional comments (15)
sleap_roots/points.py (7)

142-150: Enhanced docstring for multi-instance logic.

These lines accurately describe how node_index can be either a single integer or an array of integers. The mention of shape requirements for single vs. multiple instances is clear and aligns with the function’s implementation.


158-160: Properly documented error conditions.

The docstring clarifies when a ValueError and TypeError can be raised, ensuring future users understand the possible exceptions.


161-165: Dimension check is valid.

Raising a ValueError for invalid array dimensions ensures only 2D or 3D shapes reach further logic. This strict check helps avoid subtle shape-related errors downstream.


179-183: Single-instance shape expansion looks good.

By converting (nodes, 2) to (1, nodes, 2), you unify shapes for subsequent indexing. This approach is clean and consistent.


185-189: Node index range check is correct.

Ensuring 0 <= node_index < pts.shape[1] (or all values within that range) for both single and array-like node_index helps prevent out-of-bounds indexing.


191-202: Integer indexing logic is straightforward.

Returning the squeezed array for single-instance input vs. a 2D array for multiple instances is a clean solution to maintain consistent shapes.


203-210: Array-based indexing aligns with multi-instance usage.

Using pts[np.arange(pts.shape[0]), node_index, :] for per-instance indexing is a concise solution, ensuring each instance picks a separate node index.

sleap_roots/trait_pipelines.py (8)

68-68: Import statement for get_root_vectors is correct.

Including get_root_vectors from sleap_roots.points is necessary for the newly added vector-based traits.


948-954: Clean approach to angle calculation for distal vectors.

Using get_vector_angles_from_gravity on these newly computed distal vectors is consistent with your approach, providing an intuitive angle measure.


965-973: Vector computation for proximal nodes is consistent.

get_root_vectors usage mirrors the distal node approach, maintaining consistency and readability.


975-983: Proximal angles calculation is aligned.

This parallels the distal logic, ensuring consistent angle outputs for the proximal end.


1042-1049: Primary proximal root vector logic looks correct.

Leveraging get_root_vectors with base and proximal points maintains consistent usage patterns across all primary-lateral references.


1079-1086: Consistent usage of get_root_vectors for distal side.

No immediate issues: the logic aligns with how proximal vectors are handled, ensuring synergy in your pipeline.


1088-1094:

❓ Verification inconclusive

Confirm returned shape aligns with scalar=True.

The docstring references an array of angles, but scalar=True indicates a single aggregated value. Verify whether these angles should remain per instance or be condensed.


🏁 Script executed:

#!/bin/bash
# Examine references to "primary_angle_distal" to see if tests or usage
# expect an array or a single scalar.

rg -A 5 "primary_angle_distal" tests

Length of output: 65573


Action: Verify the output shape for “primary_angle_distal”.

The tests indicate that “primary_angle_distal” is used as a singular column in the output CSV and is obtained via a direct call to get_vector_angles_from_gravity(). However, the trait’s documentation currently describes the value as “Array of primary distal angles in degrees (instances,)”, which conflicts with the scalar=True flag that suggests an aggregated (single) value per instance.

  • Please confirm that the implementation of get_vector_angles_from_gravity returns a scalar when used with scalar=True.
  • If the output is indeed a scalar, update the docstring to reflect that it yields a single value per instance rather than an array.
  • Otherwise, consider whether the scalar flag should be removed if the intended behavior is to return an array.

939-946:

❓ Verification inconclusive

Validate shape alignment between base points and distal node points.

If lateral_distal_node_pts ends up as (instances, 1, 2) instead of (instances, 2) for single-node indexing, get_root_vectors may need consistent shape inputs. Confirm that no shape mismatch arises here.


🏁 Script executed:

#!/bin/bash
# Search for usage or test references to "lateral_distal_root_vectors" 
# to see if shapes are verified or tested.

rg -A 8 "lateral_distal_root_vectors" tests

Length of output: 1438


Action Required: Confirm Consistent Input Shapes in lateral_distal_root_vectors

The trait definition in sleap_roots/trait_pipelines.py (lines 939–946) expects the output from get_root_vectors to be an array of shape (instances, 2). However, if the input trait lateral_distal_node_pts is provided with a shape of (instances, 1, 2) (as can occur in single-node indexing), it could lead to inconsistent behavior in get_root_vectors.

  • Verify that get_root_vectors properly handles and, if necessary, reshapes its inputs so that regardless of the input (whether it comes in as (instances, 2) or (instances, 1, 2)), the output is consistently (instances, 2).
  • In the tests (see tests/test_trait_pipelines.py), the usage of lateral_distal_root_vectors does not explicitly validate the shape of the processed vectors. Please consider adding test cases or assertions to ensure that when a single-node indexing scenario occurs, no extra dimension is introduced.

@andrewjoc andrewjoc closed this Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor angle function for DicotPipeline
2 participants
0