Merged
Conversation
Introduced `get_distance_other_surface_at_points_along_unit_vector` to compute distances from a surface to another along a specified unit vector. The function includes parameters for ray casting length and handling cases with no intersection. Updated documentation for clarity.
…tionality Created new helper function: _get_distance_with_directions This iterates over points and a direction vector per point to project rays along the vector, detect intersection(s) and return the distances. This is then wrapped by two other functions that project vectors normal to the surface, and project vectors using a fixed direction.
- Introduced MeniscusMesh class for creating and analyzing meniscus meshes, including extrusion and coverage metrics.... this MeniscusMesh class isnt really used. Maybe remove? or figure out if/how should update it individually?
- Updated the mesh module to import MeniscusMesh and integrate it with existing functionality.
- Added new functions for computing meniscal extrusion and meniscal coverage
- Enhanced BoneMesh class to support meniscal outcomes computation and caching.
- Directly call the new meniscal analysis functions to get extrusion/coverage.
- Added tests for meniscal coverage and extrusion calculations, both individually as well as in the BoneMesh class.
- Bumped minimum required Python version from 3.7 to 3.9 in pyproject.toml. - Dropped support for Python 3.7 and 3.8 in build configurations due to end-of-life status and compatibility issues. - Updated GitHub Actions workflows to use macOS 14 and removed deprecated macOS 13, ensuring compatibility with Apple Silicon and improved testing across Python versions 3.9 to 3.12.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements meniscal analysis functionality for computing meniscal extrusion (how far the meniscus extends beyond cartilage) and coverage (percentage of cartilage covered by meniscus). The implementation includes:
- New
MeniscusMeshclass and analysis functions inmesh_meniscus.py - Enhanced
BoneMeshclass with meniscal analysis methods and properties - Convenience API using
dict_cartilage_labelsto reduce repetitive parameter passing - Test suite with comprehensive tests including synthetic shift validation
- Updated Python version requirements (dropped 3.7-3.8 support)
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
pymskt/mesh/mesh_meniscus.py |
New module implementing meniscal analysis functions and MeniscusMesh class |
pymskt/mesh/meshes.py |
Added meniscal analysis methods to BoneMesh, dict_cartilage_labels support, lazy-evaluation properties |
pymskt/mesh/meshTools.py |
Refactored distance calculation to support custom direction vectors for IS-axis ray casting |
testing/mesh/meshMeniscus/compute_meniscal_extrusion_test.py |
Comprehensive test suite validating extrusion calculations with synthetic shifts |
testing/mesh/meshMeniscus/compute_meniscal_coverage_test.py |
Placeholder tests for coverage computation (TODO) |
testing/mesh/meshMeniscus/MeniscusMesh_create_test.py |
Placeholder tests for MeniscusMesh class (TODO) |
pyproject.toml |
Updated Python version requirements (>=3.9, dropped 3.7-3.8) |
.github/workflows/ |
Updated CI to use macos-14 runners and Python 3.9-3.12 |
README.md |
Added meniscal analysis usage example |
Comments suppressed due to low confidence (2)
pymskt/mesh/mesh_meniscus.py:1
- Using assertions for input validation in production code is not recommended. Assertions can be disabled with Python's
-Oflag. Replace with explicitifcheck and raiseValueErrorinstead.
"""
pymskt/mesh/meshes.py:1
- [nitpick] The order of values extracted from
dict_cartilage_labelsis not guaranteed in Python <3.7 (though dicts are ordered in 3.7+). Since the minimum version is now 3.9, this is fine, but consider documenting that the order matches insertion order or using explicit ordering (e.g., sorted keys) if specific ordering is required.
import os
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1399
to
+1401
| copy_._meniscus_meshes = self._meniscus_meshes.copy() | ||
| copy_._meniscal_outcomes = self._meniscal_outcomes | ||
| copy_._meniscal_cart_labels = self._meniscal_cart_labels |
There was a problem hiding this comment.
Shallow copy is performed for _meniscus_meshes dictionary, but _meniscal_outcomes and _meniscal_cart_labels are simply assigned by reference. If these contain mutable objects (dictionaries), modifications to the copy will affect the original. Consider whether deep copying is needed for these attributes as well, consistent with the deep parameter.
Owner
Author
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…_measures Merging server development with mac development.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add new MeniscusMesh class and functions to compute meniscal extrusion and meniscal coverage.
Coverage uses the bone surface. It gets the area where cartilage is above the tibia bone. Then it gets the sub-area of the cartilage where the meniscus is above. It can report these individual areas, or the percent coverage (how much of cartilage is covered in meniscus).
Extrusion gets the middle x% (10% by default) of the med or lat tibial cartilage. In this region, it gets the most medial (or lateral) edge of the cartilage projected onto the bone. It then in the sam region, gets the most medial (or lateral) meniscus. It uses the difference between men/cart to compute extrusion.
This can also use an atlas segmentation/labelmap to make it more robust (for the cartialge regions).