feat: Add HierarchicalShapeModel for surface roughness representation
#53
Conversation
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
- Introduce AbstractShapeModel as base type for all shape models - Add HierarchicalShapeModel to support surface roughness models - Implement basic operations: add_roughness_model\!, get_roughness_model, get_roughness_model_scale, has_roughness - Add placeholder coordinate transformation functions (to be implemented) - Update ShapeModel to inherit from AbstractShapeModel This is the foundation for v0.5.0's advanced surface modeling features. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add coordinate transformation functions between global and local systems - Change get_roughness_model_scale to return Float64 (1.0 if no roughness) - Add LOCAL_CENTER_OFFSET constant for UV coordinate mapping - Implement geographic convention (X: East, Y: North, Z: Up) - Use consistent variable naming (p_global, p_local, v_global, v_local) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add transform_physical_vector_global_to_local for force/torque transformations - Add transform_physical_vector_local_to_global for force/torque transformations - Enhance docstrings for all transformation functions with detailed arguments/returns - Add implementation notes about affine transformation considerations - Clarify distinction between geometric vectors (with scaling) and physical vectors 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove outdated reference to stored transformation matrices - Clarify that only scale factors are stored - Add note about on-the-fly coordinate transformation computation - Update description to be more accurate about surface roughness models 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Include hierarchical_shape_model.jl in module - Export HierarchicalShapeModel type and all public functions - Export AbstractShapeModel base type - Update module documentation to include new types - Export coordinate transformation functions for hierarchical models 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
MasanoriKanamaru
changed the title
HierarchicalShapeModel for multi-scale surface representation
MasanoriKanamaru
changed the title
HierarchicalShapeModel for multi-scale surface representationHierarchicalShapeModel for surface roughness representation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
===========================================
- Coverage 91.01% 73.54% -17.47%
===========================================
Files 14 15 +1
Lines 501 620 +119
===========================================
Hits 456 456
- Misses 45 164 +119 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Change from per-model scales to per-face scales (face_roughness_scales) - Add clear_roughness_models\! functions for clearing roughness assignments - Rename functions to plural form (add_roughness_models\!, clear_roughness_models\!) - Reorder function arguments for consistency: (hier_shape, roughness_model, scale, face_idx) - Rename has_roughness to has_roughness_model for clarity - Update get_roughness_model_scale to return 0.0 when no roughness model - Improve memory management by removing unused models in clear_roughness_models\! This allows the same ShapeModel to be shared across multiple faces with different scales, significantly improving memory efficiency for large-scale models. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add CoordinateTransformations.jl dependency (v0.6) to Project.toml - Add face_roughness_transforms field to store per-face affine transformations - Switch to identity-based initialization (scale=1.0, identity transforms) - Define transformation constants (IDENTITY_MATRIX_3x3, ZERO_VECTOR_3, IDENTITY_AFFINE_MAP) - Update clear_roughness_models\! to reset to identity instead of zero - Update documentation to reflect new identity-based approach This prepares for pre-computed coordinate transformations while maintaining backward compatibility. The identity-based approach is mathematically cleaner and avoids special cases for zero values. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Replace fill\! with broadcast assignment (.=) for consistency - Replace for loops with broadcast operations where applicable - Simplify conditional index updates using mask-based broadcasting This makes the code more concise and idiomatic Julia style. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…rguments - Remove default values for scale and transform parameters - Add trailing commas in function signatures for consistency - Add clarifying comments in clear_roughness_models\! This change requires users to explicitly specify scale and transform values, preventing accidental use of defaults during the experimental phase of the API. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…e clarity - Reorganize sections: Display Functions, Roughness Model Accessors, and Roughness Model Management for better logical grouping - Move has_roughness_model before get_roughness_model and use it internally - Add get_roughness_model_transform function and export it - Improve Base.show output to display face roughness statistics - Update documentation to reflect AffineMap usage - Fix get_roughness_model_scale docstring (returns 1.0, not 0.0) - Simplify compute_local_transform by removing unnecessary scale check This improves code organization and makes the API more consistent. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ransformations - Update field documentation to specify transformation direction - Add note in implementation section explaining the role of transforms - Clarify docstrings for get_roughness_model_transform - Update keyword argument descriptions for add_roughness_models\! - Improve field alignment in HierarchicalShapeModel docstring This ensures users understand the transformation direction correctly. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…hness_transforms - Rename compute_local_transform to compute_global_to_local_affine - Add scale as keyword argument to compute_global_to_local_affine - Update all transformation functions to use face_roughness_transforms directly - Point transformations now use the stored AffineMap directly - Vector transformations use the linear part of the AffineMap - Physical vector transformations extract pure rotation (removing scale) This completes the migration to using CoordinateTransformations.jl's AffineMap type for storing complete transformations per face. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Refactored compute_global_to_local_affine to correctly leverage the equivalence between active and passive transformations in CoordinateTransformations.jl. Key insights: - Active local-to-global transformation = Passive global-to-local transformation - No inverse computation needed due to this equivalence - Simplified implementation by building the transformation in the natural order The implementation now: 1. Builds active local-to-global transformation: - Offset from UV center [0.5, 0.5] to origin - Scale from local units to global units - Rotate from local frame to global frame (columns = local basis vectors) - Translate from origin to face center 2. Returns this transformation directly as it serves as the passive global-to-local coordinate system transformation This ensures correct mapping where: - Face center (global) ↔ UV [0.5, 0.5] (local) - Proper coordinate system transformation semantics 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…al_affine docstring Updated the docstring to clearly explain the implementation approach: - Leverages equivalence between active and passive transformations - Builds active local-to-global transformation - Returns it as passive global-to-local transformation - No inverse computation needed This clarifies the seemingly counterintuitive approach where we build a local-to-global transformation to achieve global-to-local conversion. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ntation - Renamed function from compute_global_to_local_affine to compute_face_roughness_transform - Added explicit variable passive_global_to_local to clarify active/passive equivalence - Updated docstring to emphasize the function's purpose The implementation now clearly shows: 1. Build active local-to-global transformation 2. This is equivalent to passive global-to-local transformation 3. Return it directly (no inverse needed) This makes the non-intuitive active/passive equivalence more explicit and easier to understand for future maintainers. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Major improvements:
1. Type alias for better readability:
- Added AFFINE_MAP_TYPE for AffineMap{SMatrix{3, 3, Float64, 9}, SVector{3, Float64}}
- Updated all occurrences throughout the file
2. Enhanced add_roughness_models\! API:
- All-faces version: removed transform parameter (always auto-computed)
- Both versions: scale now defaults to 1.0
- Face-specific version: transform is optional (default: nothing)
3. Code quality improvements:
- Use eachindex for more idiomatic iteration
- Updated docstrings to reflect new signatures
- Simplified transform handling logic
- Consistent spacing in type parameters for better readability
This makes the API cleaner and more user-friendly while improving
code maintainability through the type alias.
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
…okup Replaced manual loop with a more idiomatic Julia approach: - Uses `findfirst` to search for existing model - Uses `something` for the "find or add" pattern - Maintains type stability (roughness_idx is always Int) - Added comment explaining lazy evaluation behavior The lazy evaluation ensures push\! only happens when the model doesn't already exist in the collection. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ctor Renamed functions for clarity: - transform_vector_global_to_local → transform_geometric_vector_global_to_local - transform_vector_local_to_global → transform_geometric_vector_local_to_global This naming better distinguishes between: - Geometric vectors: Apply both rotation and scaling (displacements, velocities) - Physical vectors: Apply rotation only to preserve magnitude (forces, torques) Additional improvements: - Added section headers for transformation groups - Improved variable naming consistency (p_global/p_local, v_global/v_local) - Enhanced comments for clarity - Updated docstring cross-references - Refined code structure for better readability
Fixed critical bug where scale was incorrectly multiplied instead of divided when extracting the rotation matrix from the linear transformation. Since transform.linear = rotation * scale, dividing by scale correctly extracts the pure rotation matrix. The previous implementation would incorrectly apply rotation * scale², breaking physical vector transformations. Both transform_physical_vector_global_to_local and transform_physical_vector_local_to_global now correctly divide by scale.
Major documentation improvements: - Added type annotations to all function arguments and return values in docstrings - Cleaned up file-level comment, removing redundant implementation details - Updated HierarchicalShapeModel docstring to reflect current implementation - Removed outdated Future Considerations section (already implemented) - Simplified example code by removing unnecessary transform parameter - Clarified return value descriptions with explicit mention of fallback behavior - Standardized docstring formatting across all functions - Added (not scaled) clarification to physical vector return types
- Added @ref to compute_face_roughness_transform mentions in docstrings - Reorganized exports into logical groups (accessors, management, transformations) - Kept compute_face_roughness_transform as internal function (not exported) - This ensures proper documentation generation while maintaining clean API
- Added HierarchicalShapeModel to types.md - Created new sections in roughness.md for hierarchical shape model functions - Added v0.5.0 new features section to migration guide - Documented all roughness model management and coordinate transformation functions - This ensures complete documentation coverage for the new feature
- Added comprehensive entry for new hierarchical shape model functionality - Listed all new types and functions with descriptions - Added clarifications for transformation types (full affine, with scaling, rotation only) - Added separator lines between version sections for better readability - Follows existing CHANGELOG format and conventions
- v0.4.2: Changed from 2025-01-21 to 2025-07-21 - v0.4.0: Changed from 2025-07-07 to 2025-07-08 Both dates now match actual git tag dates
Added section breaks.
- Add AbstractShapeModel to API documentation - Remove @ref from internal function compute_face_roughness_transform 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Use Ref(IDENTITY_AFFINE_MAP) for broadcasting AffineMap to array - This fixes the MethodError when trying to broadcast a single AffineMap object 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ShapeModel - Add scale validation for add_roughness_models\! functions - Add method forwarding for ShapeModel interface compatibility: - Shape metrics: polyhedron_volume, equivalent_radius, etc. - Face properties: get_face_nodes - Ray intersection: all variants of intersect_ray_shape - Illumination: isilluminated, update_illumination\! - Utility functions: build_bvh\!, build_face_visibility_graph\!, etc. - Add external constructor to create HierarchicalShapeModel from nodes/faces - This ensures HierarchicalShapeModel can be used as a drop-in replacement for ShapeModel 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…grid - Add as_hierarchical parameter to both functions - When as_hierarchical=true, returns HierarchicalShapeModel instead of ShapeModel - Updated documentation to reflect new functionality - Improved code formatting with proper type annotations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add return type annotations to all transform functions - Change behavior to throw ArgumentError when face has no roughness model - Add consistent docstrings with Usage sections for all transform functions - Update tests to expect ArgumentError instead of identity transformation - Fix BVH construction in integration tests Breaking changes: - Coordinate transformation functions now throw ArgumentError instead of returning original vectors - This ensures explicit handling of faces without roughness models 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- `compute_face_roughness_transform` now returns the passive global→local transform by inverting the composed active local→global transform. - Previously it returned the active transform directly, causing `transform_point_global_to_local` to use the wrong direction and breaking round-trip for arbitrary points. - Update comments to reflect that inversion is required (remove the misleading “no inverse needed” note). - No changes to vector transform semantics in this commit.
Update comments
…face_roughness_transform` - Build R with columns êx, êy, êz (column‑major SMatrix) so local→global rotation is applied correctly. - Previous ordering mixed components across columns, risking inconsistencies for arbitrary points. - No API changes; improves transform correctness and aligns code with comments.
Cleanup comments
Cleanup "Point transformations" testset
Add a TODO about confirmation of outward normal vectors
Reduce the number of @test
Fix "Geometric vector transformations" testset
Fix "Physical vector transformations" testset
Fix scaling physical vector transformation
- Instantiate test shapes within each test set to remove hidden dependencies and improve readability. - No functional changes; tests only.
Fix "Coordinate system properties" testset
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.
Summary
This PR introduces
HierarchicalShapeModel, a new shape model type that supports multi-scale surface representation for asteroids. This allows detailed surface features (craters, boulders, roughness) to be added to base shape models while maintaining computational efficiency.Features
HierarchicalShapeModel <: AbstractShapeModelImplementation Details
Type Hierarchy
AbstractShapeModelas base typeShapeModelandHierarchicalShapeModelinherit from itCoordinate System Convention
Local coordinate systems follow geographic conventions:
API Functions
add_roughness_model!- Attach roughness model to a faceget_roughness_model- Retrieve roughness modelget_roughness_model_scale- Get scale factor (returns 1.0 if no roughness)has_roughness- Check if face has roughnesstransform_point_*- Point transformationstransform_vector_*- Geometric vector transformations (with scaling)transform_physical_vector_*- Physical vector transformations (no scaling)TODO
HierarchicalShapeModelRelated Issues
Part of v0.5.0 roadmap for hierarchical surface roughness models.
🤖 Generated with Claude Code