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".
![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
There was a problem hiding this comment.
Yes it is an extension but it is an extension that does provide an interface different than other places (albeit we did introduce something similar like this elsewhere early stage which I regret now).
The inhomogeneity is because now we support OctreeSpacing(1*u.mm) which is not the same with any other Pydantic model (Class(field_name1=value1, field_name2=value2, ...)).
Supporting one-off interface like this may bring us trouble in the future when there is change for this model. For example let's say we add a min_spacing field. Then in this spirit do we ask customer to write OctreeSpacing(1*u.mm, 0.1*u.mm)?
For anyone that uses IDE, the IDE will not prompt you this shortcut usage so we need to spend effort educate user that for this particular class and for only this class you can actually skip base_spacing. Otherwise this seems more like an internal knowledge providing convenience just for us developers not customers. BTW for most people using IDE they just type "b" and tab and the base_spacing should be completed by IDE anyway.
I am fine if there is customer push back saying they just hate typing/seeing "base_spacing" but until then let's just support one single interface like everywhere else.
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.