feat: Refactor core engine and add mesh fitting feature #9
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
This commit introduces a major refactoring of the core engine and adds a new mesh fitting capability. It also includes significant improvements to the development tooling and examples. Core Engine Refactoring: - Introduced `MeshConfig` and `EngineMesh` data structures to provide a more robust and explicit way of handling mesh configurations and transformations. - Refactored the simulation pipeline in `run.py` and `engine.py` to be more modular and easier to follow. - Improved path resolution and handling of settings from both CLI and YAML files. Mesh Fitting Feature: - Added a new `fitting` module to find the best-matching mesh from an HDF5 database based on a target transformation (translation, rotation). - Implemented Chamfer distance calculation to robustly compare mesh similarities. - Added a new example script (`fitting_example.py`) to demonstrate this functionality. Tooling and Examples: - Added new example scripts and corresponding YAML settings files to demonstrate core features like symmetry, rotations, and drafts. - Enhanced the `Justfile` with new commands for development (`install-dev`), running tests with coverage (`test-cov`), and managing examples (`generate-all`, `fleetmaster-all`, `clean-examples`). Fixes: - Resolved several `mypy` type errors in the test suite and CLI command files. - Fixed a `setuptools-scm` warning by removing a redundant version configuration in `pyproject.toml`. - Updated dependencies in `pyproject.toml` and `uv.lock` to resolve conflicts and ensure a stable build. - Updated `CONTRIBUTING.md` with instructions for squashing commits.feat: Refactor core engine and add mesh fitting feature This commit introduces a major refactoring of the core engine and adds a new mesh fitting capability. It also includes significant improvements to the development tooling and examples. Core Engine Refactoring: - Introduced `MeshConfig` and `EngineMesh` data structures to provide a more robust and explicit way of handling mesh configurations and transformations. - Refactored the simulation pipeline in `run.py` and `engine.py` to be more modular and easier to follow. - Improved path resolution and handling of settings from both CLI and YAML files. Mesh Fitting Feature: - Added a new `fitting` module to find the best-matching mesh from an HDF5 database based on a target transformation (translation, rotation). - Implemented Chamfer distance calculation to robustly compare mesh similarities. - Added a new example script (`fitting_example.py`) to demonstrate this functionality. Tooling and Examples: - Added new example scripts and corresponding YAML settings files to demonstrate core features like symmetry, rotations, and drafts. - Enhanced the `Justfile` with new commands for development (`install-dev`), running tests with coverage (`test-cov`), and managing examples (`generate-all`, `fleetmaster-all`, `clean-examples`). Fixes: - Resolved several `mypy` type errors in the test suite and CLI command files. - Fixed a `setuptools-scm` warning by removing a redundant version configuration in `pyproject.toml`. - Updated dependencies in `pyproject.toml` and `uv.lock` to resolve conflicts and ensure a stable build. - Updated `CONTRIBUTING.md` with instructions for squashing commits.
Contributor
Reviewer's GuideThis PR refactors the core engine by introducing explicit mesh configuration abstractions and a more modular pipeline, adds a mesh‐fitting feature leveraging Chamfer distance to select best matches from an HDF5 database, enhances development tooling and example scripts, and applies various dependency, type‐checking, and tooling fixes. Sequence diagram for mesh fitting: finding the best matching meshsequenceDiagram
participant fitting_example.py
participant fitting
participant hdf5_db
participant io
participant engine
actor User
User->>fitting_example.py: Run fitting example
fitting_example.py->>fitting: find_best_matching_mesh(hdf5_path, target_translation, target_rotation, water_level)
fitting->>hdf5_db: Open HDF5 file, read base_mesh and candidate meshes
fitting->>io: load_meshes_from_hdf5(hdf5_path, mesh_names)
io->>engine: Load mesh, attach metadata
fitting->>engine: Transform base mesh for each candidate
fitting->>engine: Cut mesh at water level
fitting->>fitting: Calculate Chamfer distance
fitting->>fitting_example.py: Return best match and distance
fitting_example.py->>User: Display best match result
ER diagram for HDF5 mesh database structure with new mesh attributeserDiagram
MESHES {
string mesh_name
float volume
float cog_x
float cog_y
float cog_z
float bbox_lx
float bbox_ly
float bbox_lz
string sha256
list translation
list rotation
list cog
binary stl_content
matrix inertia_tensor
}
CASES {
string group_name
string stl_mesh_name
matrix transformation_matrix
list cog_for_calculation
netcdf simulation_results
}
MESHES ||--o{ CASES: "mesh used in case"
Class diagram for new and refactored mesh configuration and engine typesclassDiagram
class MeshConfig {
+file: str
+translation: list[float]
+rotation: list[float]
+cog: list[float] | None
+wave_periods: float | list[float] | None
+wave_directions: float | list[float] | None
}
class EngineMesh {
+name: str
+mesh: trimesh.Trimesh
+config: MeshConfig
}
class SimulationSettings {
+base_mesh: str | None
+base_origin: list[float] | None
+stl_files: list[str | MeshConfig]
+output_directory: str | None
+output_hdf5_file: str
+wave_periods: float | list[float]
+wave_directions: float | list[float]
...
}
EngineMesh --> MeshConfig
SimulationSettings --> MeshConfig
MeshConfig <.. SimulationSettings: used in stl_files
Class diagram for new mesh fitting module and related exceptionsclassDiagram
class find_best_matching_mesh {
+hdf5_path: Path
+target_translation: list[float]
+target_rotation: list[float]
+water_level: float
+returns: tuple[str | None, float]
}
class DatabaseFileNotFoundError {
+path: Path
}
class HDF5AttributeError {
+attribute_name: str
}
class MeshLoadError {
+mesh_name: str
}
find_best_matching_mesh ..> DatabaseFileNotFoundError
find_best_matching_mesh ..> HDF5AttributeError
find_best_matching_mesh ..> MeshLoadError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The core pipeline logic in _process_single_stl, _run_pipeline_for_mesh, and process_all_cases_for_one_stl has become very parameter-heavy—consider encapsulating it in a Pipeline class or context object to group related state (e.g. origin_translation, mesh_config, settings) and reduce the number of arguments you need to pass around.
- There’s a lot of repeated HDF5 boilerplate between _handle_existing_mesh, _write_mesh_to_group, and add_mesh_to_database—think about extracting a higher-level helper or context manager for opening the file, checking/overwriting groups, and writing datasets to DRY up the code.
- The new fitting module imports private engine functions (_apply_mesh_translation_and_rotation and _prepare_capytaine_body)—it might be cleaner to expose those transformations via a public API on EngineMesh or a utilities module so the fitting code doesn’t depend on internal function names.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The core pipeline logic in _process_single_stl, _run_pipeline_for_mesh, and process_all_cases_for_one_stl has become very parameter-heavy—consider encapsulating it in a Pipeline class or context object to group related state (e.g. origin_translation, mesh_config, settings) and reduce the number of arguments you need to pass around.
- There’s a lot of repeated HDF5 boilerplate between _handle_existing_mesh, _write_mesh_to_group, and add_mesh_to_database—think about extracting a higher-level helper or context manager for opening the file, checking/overwriting groups, and writing datasets to DRY up the code.
- The new fitting module imports private engine functions (_apply_mesh_translation_and_rotation and _prepare_capytaine_body)—it might be cleaner to expose those transformations via a public API on EngineMesh or a utilities module so the fitting code doesn’t depend on internal function names.
## Individual Comments
### Comment 1
<location> `src/fleetmaster/core/engine.py:153-160` </location>
<code_context>
return transformed_mesh
+def _apply_mesh_translation_and_rotation(
+ mesh: trimesh.Trimesh,
+ translation_vector: npt.NDArray[np.float64] | list | None = None,
+ rotation_vector_deg: npt.NDArray[np.float64] | list | None = None,
+ cog: npt.NDArray[np.float64] | list | None = None,
+) -> trimesh.Trimesh:
+ """Apply a translation and rotation to a mesh object."""
+ if translation_vector is not None and isinstance(translation_vector, list):
+ translation_vector = np.array(translation_vector)
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Translation and rotation logic may not handle numpy arrays as input robustly.
Using np.asarray for translation_vector and rotation_vector_deg will handle both lists and numpy arrays correctly, preventing unintended overwrites of valid numpy array inputs.
</issue_to_address>
### Comment 2
<location> `src/fleetmaster/core/engine.py:187-188` </location>
<code_context>
+ # Apply rotation around the COG if specified
+ if has_rotation:
+ # Determine the point of rotation
+ if cog is not None:
+ if isinstance(cog, list):
+ rotation_point = np.array(cog)
+ logger.debug(f"Using specified COG {rotation_point} as rotation point.")
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential uninitialized variable 'rotation_point' if cog is not a list.
If cog is not a list, rotation_point remains undefined, which may cause a NameError. Please ensure rotation_point is initialized whenever cog is not None.
</issue_to_address>
### Comment 3
<location> `src/fleetmaster/core/engine.py:176` </location>
<code_context>
+ return mesh
+
+ # Start with an identity matrix (no transformation)
+ # The affine matrix is definets as:
+ # [ R R R T ]
+ # [ R R R T ]
+ # [ R R R T ]
+ # [ 0 0 0 S ]
+ # In our case the scaling factor always S = 1.
+ transform_matrix = np.identity(4)
+
</code_context>
<issue_to_address>
**nitpick (typo):** Typo in comment: 'definets' should be 'defined'.
Please update the comment to fix the typo.
```suggestion
# The affine matrix is defined as:
```
</issue_to_address>
### Comment 4
<location> `src/fleetmaster/core/engine.py:269-273` </location>
<code_context>
hull_mesh = cpt.ReflectionSymmetricMesh(hull_mesh, plane=cpt.xOz_Plane)
boat = cpt.FloatingBody(mesh=hull_mesh, lid_mesh=lid_mesh, center_of_mass=cog)
+ boat.keep_immersed_part(free_surface=water_level)
+
+ # Important: do this step after keep_immersed_part in order to keep the body constent with the cut mesh
</code_context>
<issue_to_address>
**suggestion:** Potential edge case if water_level is above the mesh; may result in empty mesh.
If keep_immersed_part returns an empty mesh, this may cause errors later. Please add a check or warning for empty meshes.
```suggestion
boat = cpt.FloatingBody(mesh=hull_mesh, lid_mesh=lid_mesh, center_of_mass=cog)
boat.keep_immersed_part(free_surface=water_level)
# Check for empty mesh after keep_immersed_part
if boat.mesh.vertices.size == 0 or boat.mesh.faces.size == 0:
logger.warning("Resulting mesh is empty after keep_immersed_part. Check if water_level is above the mesh.")
# Optionally, raise an exception or handle gracefully:
# raise ValueError("Empty mesh after keep_immersed_part. Cannot proceed.")
# Important: do this step after keep_immersed_part in order to keep the body constent with the cut mesh
boat.add_all_rigid_body_dofs()
```
</issue_to_address>
### Comment 5
<location> `src/fleetmaster/core/engine.py:407-416` </location>
<code_context>
+def _process_single_stl(
</code_context>
<issue_to_address>
**suggestion:** The logic for mesh selection may be confusing if both DB and STL file exist but differ.
Document or log the mesh selection priority to ensure users understand that the DB mesh is used unless overwrite_meshes is enabled.
Suggested implementation:
```python
def _process_single_stl(
mesh_config: MeshConfig,
settings: SimulationSettings,
output_file: Path,
mesh_name_override: str | None = None,
origin_translation: npt.NDArray[np.float64] | None = None,
) -> None:
"""
Checks if a mesh exists in the database. If so, uses it.
If not, generates it, saves it, and then uses it for the simulation pipeline.
Mesh selection priority:
- If a mesh exists in the database and overwrite_meshes is False, the database mesh is used.
- If overwrite_meshes is True, the mesh is regenerated from the STL file and replaces the database mesh.
- If no mesh exists in the database, the mesh is generated from the STL file and saved to the database.
This ensures that the database mesh is preferred unless the user explicitly requests to overwrite meshes.
"""
```
```python
_write_mesh_to_group(group, mesh_to_add, mesh_config, new_hash, new_stl_content)
# Log mesh selection priority for user clarity
import logging
logger = logging.getLogger(__name__)
if mesh_exists_in_db and not settings.overwrite_meshes:
logger.info(
"Mesh for '%s' found in database and overwrite_meshes is False. Using database mesh.",
mesh_config.name
)
elif settings.overwrite_meshes:
logger.info(
"overwrite_meshes is True. Regenerating mesh for '%s' from STL file and updating database.",
mesh_config.name
)
else:
logger.info(
"No mesh found in database for '%s'. Generating mesh from STL file and saving to database.",
mesh_config.name
)
```
You may need to ensure that the variables `mesh_exists_in_db` and `settings.overwrite_meshes` are available at this point in the function. If not, adjust the log statements to use the correct variable names or move them to the appropriate location after mesh selection logic.
</issue_to_address>
### Comment 6
<location> `src/fleetmaster/core/engine.py:553-562` </location>
<code_context>
+def _process_and_save_single_case(
</code_context>
<issue_to_address>
**issue (bug_risk):** Transformation matrix calculation may be incorrect if origin_translation is not a numpy array.
Ensure origin_translation is converted to a numpy array before performing subtraction with boat.center_of_mass to avoid type-related issues.
</issue_to_address>
### Comment 7
<location> `src/fleetmaster/core/engine.py:666-671` </location>
<code_context>
+ if combine_cases and result_db is not None:
+ all_datasets.append(result_db)
if combine_cases and all_datasets:
logger.info("Combining all calculated cases into a single multi-dimensional dataset.")
combined_dataset = xr.combine_by_coords(all_datasets, combine_attrs="drop_conflicts")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Potential for silent failure if all_datasets is empty and combine_cases is True.
Add a log message when all_datasets is empty and combine_cases is True to inform users that no combined dataset was created.
</issue_to_address>
### Comment 8
<location> `src/fleetmaster/core/settings.py:13-22` </location>
<code_context>
MESH_GROUP_NAME = "meshes"
+class MeshConfig(BaseModel):
+ """Configuration for a single mesh, including its path and transformation."""
+
+ file: str
+ translation: list[float] = Field(default_factory=lambda: [0.0, 0.0, 0.0])
+ rotation: list[float] = Field(
+ default_factory=lambda: [0.0, 0.0, 0.0], description="Rotation [roll, pitch, yaw] in degrees."
+ )
+ cog: list[float] | None = Field(
+ default=None, description="Center of Gravity [x,y,z] for this mesh, around which moments are calculated."
+ )
+ wave_periods: float | list[float] | None = Field(
+ default=None, description="Mesh-specific wave periods. Overrides global settings."
+ )
+ wave_directions: float | list[float] | None = Field(
+ default=None, description="Mesh-specific wave directions in degrees. Overrides global settings."
+ )
</code_context>
<issue_to_address>
**suggestion:** MeshConfig does not validate translation/rotation/cog lengths.
Please add checks to ensure these fields are always lists of length 3.
</issue_to_address>
### Comment 9
<location> `src/fleetmaster/core/settings.py:46` </location>
<code_context>
+ default=None,
+ description="A point [x, y, z] in the local coordinate system of the base_mesh that defines the world origin.",
+ )
+ stl_files: list[str | MeshConfig] = Field(description="A list of STL mesh files or mesh configurations.")
output_directory: str | None = Field(default=None, description="Directory to save the output files.")
output_hdf5_file: str = Field(default="results.hdf5", description="Path to the HDF5 output file.")
</code_context>
<issue_to_address>
**suggestion:** Allowing both str and MeshConfig in stl_files may lead to inconsistent downstream handling.
Normalize stl_files to MeshConfig objects during validation to ensure consistent handling and reduce type-checking in downstream code.
Suggested implementation:
```python
stl_files: list[MeshConfig] = Field(description="A list of STL mesh files or mesh configurations.")
```
```python
output_directory: str | None = Field(default=None, description="Directory to save the output files.")
output_hdf5_file: str = Field(default="results.hdf5", description="Path to the HDF5 output file.")
wave_periods: float | list[float] = Field(default=[5.0, 10.0, 15.0, 20.0])
@validator("stl_files", pre=True)
def normalize_stl_files(cls, v):
# Accepts a list of str or MeshConfig, returns a list of MeshConfig
if not isinstance(v, list):
raise TypeError("stl_files must be a list")
normalized = []
for item in v:
if isinstance(item, MeshConfig):
normalized.append(item)
elif isinstance(item, str):
normalized.append(MeshConfig(path=item))
else:
raise TypeError(f"stl_files items must be str or MeshConfig, got {type(item)}")
return normalized
```
- Ensure that `MeshConfig` is imported in this file if it is not already.
- If `MeshConfig` requires more than just a `path` argument for initialization, adjust the validator accordingly.
</issue_to_address>
### Comment 10
<location> `src/fleetmaster/commands/run.py:178-187` </location>
<code_context>
+def _resolve_paths_in_config(config: dict[str, Any], settings_dir: Path) -> None:
</code_context>
<issue_to_address>
**issue:** Path resolution logic may not handle nested MeshConfig objects.
Currently, MeshConfig instances in stl_files are not processed, so their file attributes remain unresolved. Please update the logic to support MeshConfig objects.
</issue_to_address>
### Comment 11
<location> `src/fleetmaster/commands/run.py:243-251` </location>
<code_context>
+ config = _load_config(settings_file, cli_args)
+
+ base_mesh_path = config.get("base_mesh")
+ if "stl_files" in config:
+ # Convert dicts to MeshConfig objects to satisfy mypy
+ new_stl_files: list[str | MeshConfig] = []
+ for item in config["stl_files"]:
+ if isinstance(item, dict):
+ new_stl_files.append(MeshConfig(**item))
+ else:
+ new_stl_files.append(str(item)) # it's a string
+ config["stl_files"] = new_stl_files
+
+ all_files_in_config = [item if isinstance(item, str) else item.file for item in config["stl_files"]]
</code_context>
<issue_to_address>
**issue:** Conversion to MeshConfig objects may not handle already-constructed MeshConfig instances.
Currently, MeshConfig instances are not handled and may be incorrectly converted. Add an isinstance(item, MeshConfig) check to avoid reconstructing existing instances.
</issue_to_address>
### Comment 12
<location> `src/fleetmaster/core/fitting.py:57-66` </location>
<code_context>
+def _find_best_fit_for_candidates(
</code_context>
<issue_to_address>
**suggestion:** Hybrid transformation logic may be non-intuitive and should be documented.
Please add documentation explaining the reasoning behind combining candidate and target translation/rotation, as this approach is unconventional and may be unclear to future maintainers.
</issue_to_address>
### Comment 13
<location> `tests/test_engine.py:148-149` </location>
<code_context>
- output_file = tmp_path / "db.h5"
- mock_mesh = MagicMock(spec=trimesh.Trimesh)
- mock_mesh.export.return_value = b"new content"
+@patch("h5py.File")
+def test_add_mesh_to_database_overwrite_warning(mock_h5py_file, caplog):
+ """Test that a warning is logged when a different mesh with the same name exists and overwrite is False."""
+ # Arrange
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for add_mesh_to_database with overwrite=True and different mesh content.
Please add a test to verify that when overwrite=True and the mesh content differs, the group is deleted and recreated as expected.
</issue_to_address>
### Comment 14
<location> `tests/test_engine.py:35` </location>
<code_context>
- assert dataset["radiating_dof"].dtype.kind == "U" # converted to string
-
-
def test_setup_output_file_no_stl_files(mock_settings):
- """Test _setup_output_file raises ValueError when no STL files are provided."""
+ """Test that _setup_output_file raises ValueError if no STL files are provided."""
</code_context>
<issue_to_address>
**suggestion (testing):** No test for _setup_output_file with stl_files as MeshConfig dicts.
Please add a test for _setup_output_file that uses stl_files as a dict or MeshConfig with a 'file' key to verify correct path resolution and output file handling.
</issue_to_address>
### Comment 15
<location> `tests/test_engine.py:184-193` </location>
<code_context>
+ assert call2.kwargs["mesh_name_override"] == "base_mesh_draft_2.5"
+
+
+@patch("fleetmaster.core.engine._prepare_trimesh_geometry")
+def test_run_simulation_batch_drafts_wrong_stl_count(mock_prepare, mock_settings):
"""Test that draft mode raises an error if more than one STL is provided."""
+ mock_mesh = MagicMock(spec=trimesh.Trimesh)
</code_context>
<issue_to_address>
**suggestion (testing):** No test for run_simulation_batch with base_origin specified.
Please add a test to confirm that when base_origin is set, origin_translation is calculated correctly and base_origin is saved in the HDF5 file attributes.
</issue_to_address>
### Comment 16
<location> `tests/test_engine.py:178-187` </location>
<code_context>
+def test_add_mesh_to_database_new(tmp_path):
</code_context>
<issue_to_address>
**suggestion (testing):** No test for add_mesh_to_database with MeshConfig attributes.
Please add a test that sets translation, rotation, and cog in MeshConfig and checks that these attributes are correctly stored in the HDF5 group.
</issue_to_address>
### Comment 17
<location> `tests/test_engine.py:204` </location>
<code_context>
+def test_process_single_stl(mock_settings):
</code_context>
<issue_to_address>
**suggestion (testing):** No test for _process_single_stl when mesh is loaded from HDF5 database.
Please add a test case that covers loading a mesh from HDF5 to ensure this code path is tested.
</issue_to_address>
### Comment 18
<location> `src/fleetmaster/core/engine.py:640` </location>
<code_context>
all_datasets = []
for water_level in water_levels:
for water_depth in water_depths:
for forward_speed in forwards_speeds:
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing triple-nested loops with itertools.product and collapsing repeated logging into a dict-driven loop to reduce boilerplate and visual complexity.
The refactor as‐is is already a huge improvement, but you can still simplify those deep, repeated nested loops and the long logging boilerplate. Here are two very small, focused tweaks that keep every feature intact but reduce visual nesting and indirection:
1) Replace your triple‐nested loops with a single `itertools.product` loop in `process_all_cases_for_one_stl` (and similarly in `_run_pipeline_for_mesh`):
```python
from itertools import product
def process_all_cases_for_one_stl(…):
all_datasets = []
for depth, level, speed in product(water_depths, water_levels, forwards_speeds):
case_params = {
"omegas": wave_frequencies,
"wave_directions": wave_directions,
"water_depth": depth,
"water_level": level,
"forward_speed": speed,
"update_cases": update_cases,
"combine_cases": combine_cases,
}
result_db = _process_and_save_single_case(
boat, engine_mesh.name, case_params, output_file, origin_translation
)
if combine_cases and result_db:
all_datasets.append(result_db)
…
```
2) Collapse repeated `logger.info("%-40s: %s", …)` into a simple dict‐driven loop in your `_log_pipeline_parameters`:
```python
def _log_pipeline_parameters(...):
entries = {
"Base STL file": engine_mesh.config.file,
"Vertices": engine_mesh.mesh.vertices.shape,
"Output file": output_file,
"Grid symmetry": settings.grid_symmetry,
# …
"Rotation Yaw [deg]": engine_mesh.config.rotation[2],
"Forward speed(s) [m/s]": forwards_speeds,
}
for key, val in entries.items():
logger.info(f"{key:<40}: {val}")
```
These two changes cut down on boilerplate, reduce cognitive load (no more three‐deep loops or repeated logger lines), and keep all existing functionality exactly the same.
</issue_to_address>
### Comment 19
<location> `examples/fitting_example.py:56` </location>
<code_context>
def run_fitting_example():
"""Runs the fitting example.
It defines a target transformation and then searches the HDF5 database
for the mesh that best matches that transformation.
"""
# Path to the HDF5 database file generated by the settings_rotations.yml example.
hdf5_path = Path(__file__).parent / "boxship.hdf5"
if not hdf5_path.exists():
logger.error(f"Database file not found at: {hdf5_path}")
logger.error("Please run 'fleetmaster -v run --settings-file examples/settings_rotations.yml' first.")
return
# The water level used for the comparison. This should match the level at which
# the meshes in the database were generated (wetted surface).
water_level = 0.0
draft = 2.0
# --- Test Case 1: A transformation that should perfectly match an existing mesh ---
# We are looking for a mesh that corresponds to a Z-translation of -1.0,
# a roll of 20 degrees, and a pitch of 20 degrees.
# The database contains 'boxship_t_1_r_20_20_00.stl' with these exact parameters.
print("\n--- Running Test Case 1: Exact Match ---")
target_translation_1 = [0.0, 0.0, -draft]
target_rotation_1 = [20.0, 20.0, 0.0] # [roll, pitch, yaw]
logger.info(f"Searching for best match for translation={target_translation_1}, rotation={target_rotation_1}...\n")
best_match_1, distance_1 = find_best_matching_mesh(
hdf5_path=hdf5_path,
target_translation=target_translation_1,
target_rotation=target_rotation_1,
water_level=water_level,
)
print("\n--- Result for Test Case 1 ---")
if best_match_1:
print(f"✅ Best match found: '{best_match_1}'")
print(f" - Minimized Chamfer Distance: {distance_1:.6f}")
print(" - Expected match: 'boxship_t_1_r_20_20_00'")
else:
print("❌ No match found.")
# --- Test Case 2: A transformation with irrelevant translations and rotations ---
# This case has the same core properties (Z-trans, X/Y-rot) as Case 1,
# but with added X/Y translation and a Z rotation (yaw).
# The optimization algorithm should ignore these and still find the same best match.
print("\n\n--- Running Test Case 2: Match with Noise ---")
target_translation_2 = [2.5, -4.2, -draft] # Added dx, dy
target_rotation_2 = [20.0, 20.0, 15.0] # Added yaw
logger.info(f"Searching for best match for translation={target_translation_2}, rotation={target_rotation_2}...\n")
best_match_2, distance_2 = find_best_matching_mesh(
hdf5_path=hdf5_path,
target_translation=target_translation_2,
target_rotation=target_rotation_2,
water_level=water_level,
)
print("\n--- Result for Test Case 2 ---")
if best_match_2:
print(f"✅ Best match found: '{best_match_2}'")
print(f" - Minimized Chamfer Distance: {distance_2:.6f}")
print(" - Expected match: 'boxship_t_1_r_20_20_00'")
print(" - Note: The distance should be very close to the distance in Case 1.")
else:
print("❌ No match found.")
# --- Test Case 3: A transformation with irrelevant translations and rotations ---
# This case has the same core properties (Z-trans, X/Y-rot) as Case 1,
# but with added X/Y translation and a Z rotation (yaw).
# this time, difference valeus for the z-translation and x,y rotation are assumed.
# the expected result should give a larger distance
print("\n\n--- Running Test Case 3: Match with Noise ---")
target_translation_3 = [2.5, -4.2, -draft * 1.2] # Added dx, dy AND dz
target_rotation_3 = [23.0, 19.0, 15.0] # Added yaw AND roll and pitch
logger.info(f"Searching for best match for translation={target_translation_3}, rotation={target_rotation_3}...\n")
best_match_3, distance_3 = find_best_matching_mesh(
hdf5_path=hdf5_path,
target_translation=target_translation_3,
target_rotation=target_rotation_3,
water_level=water_level,
)
print("\n--- Result for Test Case 3 ---")
if best_match_2:
print(f"✅ Best match found: '{best_match_3}'")
print(f" - Minimized Chamfer Distance: {distance_3:.6f}")
print(" - Expected match: 'boxship_t_1_r_20_20_00'")
print(" - Note: The distance should be larger than both case 1 and case 2.")
else:
print("❌ No match found.")
</code_context>
<issue_to_address>
**issue (code-quality):** Extract duplicate code into function ([`extract-duplicate-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-duplicate-method/))
</issue_to_address>
### Comment 20
<location> `src/fleetmaster/core/engine.py:426` </location>
<code_context>
def _process_single_stl(
mesh_config: MeshConfig,
settings: SimulationSettings,
output_file: Path,
mesh_name_override: str | None = None,
origin_translation: npt.NDArray[np.float64] | None = None,
) -> None:
"""
Checks if a mesh exists in the database. If so, uses it.
If not, generates it, saves it, and then uses it for the simulation pipeline.
"""
mesh_name = mesh_name_override or Path(mesh_config.file).stem
final_mesh_to_process: trimesh.Trimesh | None = None
# --- Workflow to determine the mesh to process ---
if not settings.overwrite_meshes:
# 1. Prioritize loading from the HDF5 database if it already exists, only if we dont want
# to overwrite the meshes
try:
existing_meshes = load_meshes_from_hdf5(output_file, [mesh_name])
if existing_meshes:
final_mesh_to_process = existing_meshes[0]
logger.info(f"Found existing mesh '{mesh_name}' in the database. Using it directly.")
except FileNotFoundError:
# The HDF5 file doesn't exist yet, so no meshes can exist. This is expected on the first run.
pass
# 2. If not in DB, check if a pre-translated STL file exists.
if final_mesh_to_process is None:
target_stl_path = Path(mesh_config.file)
if target_stl_path.exists():
logger.info(
f"Mesh '{mesh_name}' not in DB, but found STL file: '{target_stl_path}'. Loading and adding to DB."
)
# Load the existing, presumably pre-translated, STL file.
final_mesh_to_process = _prepare_trimesh_geometry(stl_file=str(target_stl_path))
else:
# 3. If neither DB entry nor STL file exists, generate the mesh.
logger.info(f"Mesh '{mesh_name}' not found in DB or as STL file. Attempting to generate it.")
# Use the global base_mesh as the source for generation.
source_file_path = settings.base_mesh
if not source_file_path or not Path(source_file_path).exists():
err_msg = (
f"Cannot generate mesh '{mesh_name}'. The source file '{target_stl_path}' does not exist, "
f"and no valid 'base_mesh' ('{source_file_path}') is configured to generate it from."
)
raise FileNotFoundError(err_msg)
# Load the base STL and apply the specified translation.
translated_mesh = _prepare_trimesh_geometry(str(source_file_path), mesh_config)
# Save the newly generated, translated mesh to a separate STL file for inspection.
logger.info(f"Saving newly generated translated mesh to: {target_stl_path}")
translated_mesh.export(target_stl_path)
final_mesh_to_process = translated_mesh
# 4. Run the complete processing pipeline with the determined mesh.
engine_mesh = EngineMesh(name=mesh_name, mesh=final_mesh_to_process, config=mesh_config)
_run_pipeline_for_mesh(engine_mesh, settings, output_file, origin_translation)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Extract code out into function ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
- Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/inline-immediately-returned-variable/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+153
to
+160
| def _apply_mesh_translation_and_rotation( | ||
| mesh: trimesh.Trimesh, | ||
| translation_vector: npt.NDArray[np.float64] | list | None = None, | ||
| rotation_vector_deg: npt.NDArray[np.float64] | list | None = None, | ||
| cog: npt.NDArray[np.float64] | list | None = None, | ||
| ) -> trimesh.Trimesh: | ||
| """Apply a translation and rotation to a mesh object.""" | ||
| if translation_vector is not None and isinstance(translation_vector, list): |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Translation and rotation logic may not handle numpy arrays as input robustly.
Using np.asarray for translation_vector and rotation_vector_deg will handle both lists and numpy arrays correctly, preventing unintended overwrites of valid numpy array inputs.
Comment on lines
+187
to
+188
| if cog is not None: | ||
| if isinstance(cog, list): |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Potential uninitialized variable 'rotation_point' if cog is not a list.
If cog is not a list, rotation_point remains undefined, which may cause a NameError. Please ensure rotation_point is initialized whenever cog is not None.
| return mesh | ||
|
|
||
| # Start with an identity matrix (no transformation) | ||
| # The affine matrix is definets as: |
Contributor
There was a problem hiding this comment.
nitpick (typo): Typo in comment: 'definets' should be 'defined'.
Please update the comment to fix the typo.
Suggested change
| # The affine matrix is definets as: | |
| # The affine matrix is defined as: |
Comment on lines
269
to
273
| boat = cpt.FloatingBody(mesh=hull_mesh, lid_mesh=lid_mesh, center_of_mass=cog) | ||
| boat.keep_immersed_part(free_surface=water_level) | ||
|
|
||
| # Important: do this step after keep_immersed_part in order to keep the body constent with the cut mesh | ||
| boat.add_all_rigid_body_dofs() |
Contributor
There was a problem hiding this comment.
suggestion: Potential edge case if water_level is above the mesh; may result in empty mesh.
If keep_immersed_part returns an empty mesh, this may cause errors later. Please add a check or warning for empty meshes.
Suggested change
| boat = cpt.FloatingBody(mesh=hull_mesh, lid_mesh=lid_mesh, center_of_mass=cog) | |
| boat.keep_immersed_part(free_surface=water_level) | |
| # Important: do this step after keep_immersed_part in order to keep the body constent with the cut mesh | |
| boat.add_all_rigid_body_dofs() | |
| boat = cpt.FloatingBody(mesh=hull_mesh, lid_mesh=lid_mesh, center_of_mass=cog) | |
| boat.keep_immersed_part(free_surface=water_level) | |
| # Check for empty mesh after keep_immersed_part | |
| if boat.mesh.vertices.size == 0 or boat.mesh.faces.size == 0: | |
| logger.warning("Resulting mesh is empty after keep_immersed_part. Check if water_level is above the mesh.") | |
| # Optionally, raise an exception or handle gracefully: | |
| # raise ValueError("Empty mesh after keep_immersed_part. Cannot proceed.") | |
| # Important: do this step after keep_immersed_part in order to keep the body constent with the cut mesh | |
| boat.add_all_rigid_body_dofs() |
Comment on lines
265
to
+416
| def _process_single_stl( | ||
| stl_file: str, settings: SimulationSettings, output_file: Path, mesh_name_override: str | None = None | ||
| mesh_config: MeshConfig, | ||
| settings: SimulationSettings, | ||
| output_file: Path, | ||
| mesh_name_override: str | None = None, | ||
| origin_translation: npt.NDArray[np.float64] | None = None, | ||
| ) -> None: | ||
| """ | ||
| Checks if a mesh exists in the database. If so, uses it. | ||
| If not, generates it, saves it, and then uses it for the simulation pipeline. |
Contributor
There was a problem hiding this comment.
suggestion: The logic for mesh selection may be confusing if both DB and STL file exist but differ.
Document or log the mesh selection priority to ensure users understand that the DB mesh is used unless overwrite_meshes is enabled.
Suggested implementation:
def _process_single_stl(
mesh_config: MeshConfig,
settings: SimulationSettings,
output_file: Path,
mesh_name_override: str | None = None,
origin_translation: npt.NDArray[np.float64] | None = None,
) -> None:
"""
Checks if a mesh exists in the database. If so, uses it.
If not, generates it, saves it, and then uses it for the simulation pipeline.
Mesh selection priority:
- If a mesh exists in the database and overwrite_meshes is False, the database mesh is used.
- If overwrite_meshes is True, the mesh is regenerated from the STL file and replaces the database mesh.
- If no mesh exists in the database, the mesh is generated from the STL file and saved to the database.
This ensures that the database mesh is preferred unless the user explicitly requests to overwrite meshes.
""" _write_mesh_to_group(group, mesh_to_add, mesh_config, new_hash, new_stl_content)
# Log mesh selection priority for user clarity
import logging
logger = logging.getLogger(__name__)
if mesh_exists_in_db and not settings.overwrite_meshes:
logger.info(
"Mesh for '%s' found in database and overwrite_meshes is False. Using database mesh.",
mesh_config.name
)
elif settings.overwrite_meshes:
logger.info(
"overwrite_meshes is True. Regenerating mesh for '%s' from STL file and updating database.",
mesh_config.name
)
else:
logger.info(
"No mesh found in database for '%s'. Generating mesh from STL file and saving to database.",
mesh_config.name
)You may need to ensure that the variables mesh_exists_in_db and settings.overwrite_meshes are available at this point in the function. If not, adjust the log statements to use the correct variable names or move them to the appropriate location after mesh selection logic.
Comment on lines
+148
to
+149
| @patch("h5py.File") | ||
| def test_add_mesh_to_database_overwrite_warning(mock_h5py_file, caplog): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Missing test for add_mesh_to_database with overwrite=True and different mesh content.
Please add a test to verify that when overwrite=True and the mesh content differs, the group is deleted and recreated as expected.
| assert dataset["radiating_dof"].dtype.kind == "U" # converted to string | ||
|
|
||
|
|
||
| def test_setup_output_file_no_stl_files(mock_settings): |
Contributor
There was a problem hiding this comment.
suggestion (testing): No test for _setup_output_file with stl_files as MeshConfig dicts.
Please add a test for _setup_output_file that uses stl_files as a dict or MeshConfig with a 'file' key to verify correct path resolution and output file handling.
Comment on lines
+184
to
+193
| @patch("fleetmaster.core.engine._prepare_trimesh_geometry") | ||
| @patch("pathlib.Path.exists", return_value=True) | ||
| def test_process_single_stl(mock_exists, mock_prepare, mock_load, mock_run_pipeline): | ||
| """Test the main processing pipeline for a single STL file.""" | ||
| stl_file = "/path/to/dummy.stl" | ||
| mesh_config = MeshConfig(file="/path/to/dummy.stl") | ||
| settings = SimulationSettings(stl_files=[mesh_config]) | ||
| output_file = Path("/fake/output.hdf5") | ||
|
|
||
| _process_single_stl(stl_file, mock_settings, output_file) | ||
| mock_mesh = MagicMock(spec=trimesh.Trimesh) | ||
| mock_prepare.return_value = mock_mesh |
Contributor
There was a problem hiding this comment.
suggestion (testing): No test for run_simulation_batch with base_origin specified.
Please add a test to confirm that when base_origin is set, origin_translation is calculated correctly and base_origin is saved in the HDF5 file attributes.
Comment on lines
+178
to
187
| name = _generate_case_group_name("mesh1", 100.0, -2.5, 5.0) | ||
| assert name == "mesh1_wd_100_wl_-2.5_fs_5" | ||
|
|
||
|
|
||
| @patch("fleetmaster.core.engine.process_all_cases_for_one_stl") | ||
| def test_process_single_stl(mock_process_all, mock_settings): | ||
| @patch("fleetmaster.core.engine._run_pipeline_for_mesh") | ||
| @patch("fleetmaster.core.engine.load_meshes_from_hdf5", return_value=[]) | ||
| @patch("fleetmaster.core.engine._prepare_trimesh_geometry") | ||
| @patch("pathlib.Path.exists", return_value=True) | ||
| def test_process_single_stl(mock_exists, mock_prepare, mock_load, mock_run_pipeline): | ||
| """Test the main processing pipeline for a single STL file.""" |
Contributor
There was a problem hiding this comment.
suggestion (testing): No test for add_mesh_to_database with MeshConfig attributes.
Please add a test that sets translation, rotation, and cog in MeshConfig and checks that these attributes are correctly stored in the HDF5 group.
| assert isinstance(engine_mesh_arg, EngineMesh) | ||
| assert engine_mesh_arg.name == "dummy" | ||
| assert engine_mesh_arg.mesh == mock_mesh | ||
| assert engine_mesh_arg.config == mesh_config |
Contributor
There was a problem hiding this comment.
suggestion (testing): No test for _process_single_stl when mesh is loaded from HDF5 database.
Please add a test case that covers loading a mesh from HDF5 to ensure this code path is tested.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This commit introduces a major refactoring of the core engine and adds a new mesh fitting capability. It also includes significant improvements to the development tooling and examples.
Core Engine Refactoring:
MeshConfigandEngineMeshdata structures to provide a more robust and explicit way of handling mesh configurations and transformations.run.pyandengine.pyto be more modular and easier to follow.Mesh Fitting Feature:
fittingmodule to find the best-matching mesh from an HDF5 database based on a target transformation (translation, rotation).fitting_example.py) to demonstrate this functionality.Tooling and Examples:
Justfilewith new commands for development (install-dev), running tests with coverage (test-cov), and managing examples (generate-all,fleetmaster-all,clean-examples).Fixes:
mypytype errors in the test suite and CLI command files.setuptools-scmwarning by removing a redundant version configuration inpyproject.toml.pyproject.tomlanduv.lockto resolve conflicts and ensure a stable build.CONTRIBUTING.mdwith instructions for squashing commits.feat: Refactor core engine and add mesh fitting featureThis commit introduces a major refactoring of the core engine and adds a new mesh fitting capability. It also includes significant improvements to the development tooling and examples.
Core Engine Refactoring:
MeshConfigandEngineMeshdata structures to provide a more robust and explicit way of handling mesh configurations and transformations.run.pyandengine.pyto be more modular and easier to follow.Mesh Fitting Feature:
fittingmodule to find the best-matching mesh from an HDF5 database based on a target transformation (translation, rotation).fitting_example.py) to demonstrate this functionality.Tooling and Examples:
Justfilewith new commands for development (install-dev), running tests with coverage (test-cov), and managing examples (generate-all,fleetmaster-all,clean-examples).Fixes:
mypytype errors in the test suite and CLI command files.setuptools-scmwarning by removing a redundant version configuration inpyproject.toml.pyproject.tomlanduv.lockto resolve conflicts and ensure a stable build.CONTRIBUTING.mdwith instructions for squashing commits.Summary by Sourcery
Refactor the core engine for modular mesh processing and configuration, add a new mesh fitting feature using Chamfer distance, enhance CLI and YAML handling with MeshConfig, and update examples, tooling, and tests accordingly
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: