Conversation
…pacing-for-volume-mesh rebase on master
shreyas-flex
requested review from
benflexcompute,
piotrkluba and
savithru-flex
…pacing-for-volume-mesh
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9e405ba79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if param_info.project_length_unit is None: | ||
| add_validation_warning( | ||
| "No project length unit found; `octree_spacing` will not be set automatically. " | ||
| "Octree spacing validation will be skipped." | ||
| ) |
There was a problem hiding this comment.
This validator emits a warning whenever project_length_unit is absent, even when param_info.is_beta_mesher is False and octree spacing is irrelevant for the legacy mesher. In non-beta validation flows, this introduces a new spurious warning ("octree_spacing ... will not be set automatically") that can pollute validate_params_with_context warning output and force callers/tests to provide a fake project length unit just to keep warning-free runs. The warning/defaulting branch should be gated by beta-mesher context so legacy workflows are unaffected.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if isinstance(refinement, UniformRefinement): | ||
| check_spacing(refinement.spacing, type(refinement).__name__) | ||
|
|
||
| return self |
There was a problem hiding this comment.
Duplicated validator logic across two class pairs
Low Severity
The _check_sizing_against_octree_series method (including the inner check_spacing closure) is copy-pasted between MeshingParams and VolumeMeshingParams — only the warning message string differs. Similarly, _set_default_octree_spacing is identically duplicated between MeshingDefaults and VolumeMeshingDefaults. The check_spacing logic also closely mirrors the one in snappy's _check_sizing_against_octree_series. A shared utility (or a method on OctreeSpacing itself) would eliminate these three copies, reducing the risk of inconsistent bug fixes.
Additional Locations (2)
| octree_spacing: Optional[OctreeSpacing] = pd.Field( | ||
| None, validation_alias=AliasChoices("octree_spacing", "base_spacing") | ||
| ) |
There was a problem hiding this comment.
Why not
octree_spacing: Optional[OctreeSpacing] = pd.Field( None, alias="base_spacing") )
| refinements: Optional[List[SnappySurfaceRefinementTypes]] = pd.Field(None) | ||
| base_spacing: Optional[OctreeSpacing] = pd.Field(None) | ||
| octree_spacing: Optional[OctreeSpacing] = pd.Field( | ||
| None, validation_alias=AliasChoices("octree_spacing", "base_spacing") |
There was a problem hiding this comment.
validation_alias = "base_spacing" Is good enough?
|
|
||
| @pd.model_validator(mode="before") | ||
| @classmethod | ||
| def _project_spacing_to_object(cls, input_data): |
There was a problem hiding this comment.
IIRC I had a comment when Snappy introduces this function. I understand this provides convenience but at the same time introduces inhomogeneity on interface.
Personally it is more of a burden/annoyance to me remembering here I have a shortcut OctreeSpacing(1*u.mm) than just typing "base_spacing = " like everywhere else.
I wish we can get rid of this before it goes public. CC:@piotrkluba
There was a problem hiding this comment.
Why does it provide inhomogeneity? The goal was for the user to be able to specify the spacing with The OctreeSpacing object as well as a simple length value. For me its an extension of the interface for convenience
3 participants
2**n * base_spacingfor some integer n).flow360.MeshingDefaultsis constructed.WIP:
Note
Medium Risk
Touches meshing parameter validation and translation output (including emitted JSON), which can change runtime mesher behavior and produce new warnings, but is largely additive and backward-compatible via aliasing.
Overview
Adds first-class
octree_spacingsupport to the non-snappy (volume) meshing workflow by introducing anOctreeSpacingconfig onMeshingDefaults/VolumeMeshingDefaults, defaulting it fromproject_length_unitwhen available and warning when missing.Renames snappy’s
base_spacinginput tooctree_spacing(keeps backward-compatible alias + deprecation warning), updates translators to consume the new field, and propagatesoctreeBaseSpacinginto the volume mesher farfield JSON when using the in-house mesher.Adds beta-mesher validation that warns when
UniformRefinement.spacingis not aligned to the octree power-of-2 spacing series, and expands unit tests/fixtures and golden JSON accordingly (including ensuringAssetCachecarriesproject_length_unit).Written by Cursor Bugbot for commit d071510. This will update automatically on new commits. Configure here.