-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
WalkthroughThe pull request introduces modifications across multiple modules to enhance vector and node handling. In Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 #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. 🚀 New features to boost your workflow:
|
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.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
sleap_roots/points.py (1)
222-245
: 🛠️ Refactor suggestionVector definition conflicts with function description.
Per the docstring, “the vector from start to end” implies
end_nodes - start_nodes
, but the code doesstart_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 unusedRemove 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 beTypeError
. 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
📒 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
andget_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 applyingget_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
withequal_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, whilepts_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
withequal_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 ofget_root_angle
, leveraging the precomputed vectors. This approach:
- Improves code structure by separating concerns
- Handles near-zero vectors more consistently (marks them as NaN)
- 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.
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.
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) |
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.
after this shouldn't dimension of arrays both be 2? can we use that to simplify the logic?
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.
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 usingget_root_vectors
andget_vector_angles_from_gravity
. This improves modularity and clarity in the code by:
- First extracting node points
- Then computing vectors between points
- 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
📒 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
includingget_nodes
andget_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
withget_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:
- Extract node points using
get_nodes
- Calculate vectors between points using
get_root_vectors
- 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 forget_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 forget_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 ofget_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. Settinginclude_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 withget_vector_angles_from_gravity
instead of the previousget_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
andend_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 nodesThis matches the expectation of
get_root_vectors
which calculates from start to end.Also applies to: 965-973, 1041-1049, 1078-1086
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.
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)
, iflateral_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 toget_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
📒 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
andTypeError
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-likenode_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 forget_root_vectors
is correct.Including
get_root_vectors
fromsleap_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 ofget_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" testsLength 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 thescalar=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 withscalar=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" testsLength 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 fromget_root_vectors
to be an array of shape(instances, 2)
. However, if the input traitlateral_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 inget_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 oflateral_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.
get_root_angle
withget_root_vectors
andget_vector_angles_from_gravity
in DicotPipline by adding new trait definitionstest_trait_pipelines.py
DicotRootAngle.ipynb
explaining new method for calculating root anglesFixes #97
Summary by CodeRabbit
New Features
Tests