-
Notifications
You must be signed in to change notification settings - Fork 21
Add Custom Grid Parameter to Geometry Class and Discretization #518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devel
Are you sure you want to change the base?
Conversation
- Add grid parameter to Geometry class constructor and properties - Update discretize_space function to use custom grid from domain_h when available - Fallback to uniform grid when no custom grid is provided - Support for 1D and 2D domains with custom breakpoint grids
- Add 15 new security validation tests for grid parameter - Test grid type validation (list/tuple required) - Test grid dimension consistency with domain - Test grid length consistency with ncells - Test monotonic ordering of grid points - Test boundary value matching with tolerance - Add parametrized tests for all error conditions - Test security in 1D, 2D, and 3D configurations - Ensure security checks don't interfere with valid functionality - Total tests: 39 (24 existing + 15 new security tests) These tests ensure robust validation of custom grids preventing: - Invalid grid types and formats - Dimension mismatches between grid and domain - Inconsistent ncells vs grid length - Non-monotonic grid points - Boundary value mismatches - Numerical precision issues All tests pass successfully, confirming grid security implementation works correctly while preserving existing functionality.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
|
Hi Arasu, thanks for the nice changes! I had a quick look and noticed one thing:
It would be very nice, if you could add the psydac/psydac/api/discretization.py Line 556 in e47ff5d
as well (and the function it is calling). Unrelated to this PR, I just realized that |
- Add grid parameter to discretize_domain() in discretization.py - Add grid parameter to Geometry.from_topological_domain() in geometry.py
|
Thank you @FrederikSchnack for your comments. I've added the psydac/psydac/api/discretization.py Line 556 in e79a11c
and to Geometry._from_topological_domain inLine 164 in e79a11c
I’ve also updated the test file accordingly. |
Do you mean Is there an option to only pass the grid along one axis, e.g. with |
Psydac handles spline/NURBS (discrete) mappings quite differently from analytical mappings. Discrete mappings are handled through the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @ArasuCandassamy!
I have a few comments which you will find below. I was also thinking that it would be nice to allow the user to specify the grid just for some of the directions. For example, this could be done with the call
grid_y = [0, 0.1, 0.2, 0.333, 0.444, 0.5]
domain_h = discretize(domain, ncells=[4, 5, 6], grid=[None, grid_y, None])where grid=[None, grid_y, None] means that the grid is only specified along the axis of index 1.
psydac/api/discretization.py
Outdated
| if hasattr(domain_h, 'grid') and domain_h.grid is not None: | ||
| # Use provided grid of breakpoints | ||
| grids = domain_h.grid | ||
|
|
||
| # Safety checks for grid consistency | ||
| if not isinstance(grids, (list, tuple)): | ||
| raise TypeError(f"Grid must be a list or tuple, got {type(grids)}") | ||
|
|
||
| if len(grids) != len(ncells): | ||
| raise ValueError(f"Grid dimensions ({len(grids)}) must match domain dimensions ({len(ncells)})") | ||
|
|
||
| for dim, (grid, nc, xmin, xmax) in enumerate(zip(grids, ncells, min_coords, max_coords)): | ||
| grid = np.asarray(grid) | ||
|
|
||
| # Check grid length vs ncells consistency | ||
| expected_grid_length = nc + 1 | ||
| if len(grid) != expected_grid_length: | ||
| raise ValueError(f"Dimension {dim}: grid length ({len(grid)}) must be ncells+1 ({expected_grid_length})") | ||
|
|
||
| # Check if grid is sorted | ||
| if not np.all(np.diff(grid) > 0): | ||
| raise ValueError(f"Dimension {dim}: grid points must be strictly increasing") | ||
|
|
||
| # Check domain boundaries (with tolerance for numerical precision) | ||
| tol = 1e-12 | ||
| if abs(grid[0] - xmin) > tol: | ||
| raise ValueError(f"Dimension {dim}: grid start ({grid[0]}) must match domain minimum ({xmin})") | ||
| if abs(grid[-1] - xmax) > tol: | ||
| raise ValueError(f"Dimension {dim}: grid end ({grid[-1]}) must match domain maximum ({xmax})") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that these checks should be made directly in the constructor of the Geometry class, not here. Here instead you should check that if the knots are passed, these are consistent with domain_h.ncells and domain_h.grid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed these checks from there and I have put them in geometry.py with a function in the Geometry class:
Line 338 in 6e90be7
| def _validate_grid_consistency(self, domain, ncells, grid): |
psydac/cad/geometry.py
Outdated
| self._mappings = mappings | ||
| self._cart = None | ||
| self._is_parallel = comm is not None | ||
| self._grid = grid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before storing the input parameter grid, there are a few things that we should verify:
- If
filenameis provided, one cannot passncellsorgrid. I would avoid more complicated solutions. - If
ncellsorgridis provided, one cannot passfilename. - Since
ncellsandgridare dictionaries, they must have for keys the names of the domain patches, i.e.domain.keys()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen that the verification is done in the discretize_domain function here :
psydac/psydac/api/discretization.py
Lines 556 to 571 in e79a11c
| def discretize_domain(domain, *, filename=None, ncells=None, periodic=None, comm=None, mpi_dims_mask=None, grid=None): | |
| if comm is not None: | |
| # Create a copy of the communicator | |
| comm = comm.Dup() | |
| if not (filename or ncells): | |
| raise ValueError("Must provide either 'filename' or 'ncells'") | |
| elif filename and ncells: | |
| raise ValueError("Cannot provide both 'filename' and 'ncells'") | |
| elif filename: | |
| return Geometry(filename=filename, comm=comm) | |
| elif ncells: | |
| return Geometry.from_topological_domain(domain, ncells, periodic=periodic, comm=comm, mpi_dims_mask=mpi_dims_mask, grid=grid) |
Can I add the checks on
grid, filenameand ncells here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know that the same checks are done in discretize_domain...😅 Nevertheless a class constructor should always check its arguments, and now is a good time to fix it!😉
psydac/cad/geometry.py
Outdated
| grid: list of breakpoints | ||
| The grid of breakpoints in each direction. | ||
| If not given, the grid will be constructed from the ncells and periodicity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This argument has to be a dictionary just like ncells, periodic, and mappings. I think the docstring has to be updated regarding ncells and periodic: neither a list nor a tuple is accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new docstring is:
grid: list | tuple | dict
The grid of breakpoints in each direction.
If not given, the grid will be constructed from the ncells and periodicity.
So, it's possible to give a list or a tuple to create a Geometry class object, but Geometry.from_topological_domain is converting tuples and lists into a dict:
Lines 229 to 230 in 6e90be7
| if isinstance(grid, (list, tuple)): | |
| grid = {itr.name:grid for itr in interior} |
| @classmethod | ||
| def from_topological_domain(cls, domain, ncells, *, periodic=None, comm=None, mpi_dims_mask=None): | ||
| def from_topological_domain(cls, domain, ncells, *, periodic=None, comm=None, mpi_dims_mask=None, grid=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry that this function does not have a docstring. In practice it is a helper constructor which discretizes a multipatch domain using the same discretization parameters for all patches, i.e. the same number of cells, periodicity, and (now) grid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a docstring for this function:
Lines 174 to 207 in 6e90be7
| def from_topological_domain(cls, domain, ncells, *, periodic=None, comm=None, mpi_dims_mask=None, grid=None): | |
| """ | |
| Create a Geometry object from a topological domain and discretization parameters. | |
| Parameters | |
| ---------- | |
| cls : type | |
| The class type (typically `Geometry`) to instantiate. | |
| domain : Sympde.topology.Domain | |
| The symbolic domain to be discretized. | |
| ncells : list | tuple | dict | |
| The number of cells of the discretized topological domain in each direction. | |
| periodic : list | tuple | dict | |
| The periodicity of the topological domain in each direction. | |
| comm: MPI.Comm | |
| MPI intra-communicator. | |
| mpi_dims_mask: list of bool | |
| True if the dimension is to be used in the domain decomposition (=default for each dimension). | |
| If mpi_dims_mask[i]=False, the i-th dimension will not be decomposed. | |
| grid: list | tuple | dict | |
| The grid of breakpoints in each direction. | |
| If not given, the grid will be constructed from the ncells and periodicity. | |
| Returns | |
| ------- | |
| geo : Geometry | |
| The constructed Geometry object. | |
| """ |
- Add consistency checks in geometry.py - Pass grid as a dictionary - Add docstrings - Add support for partially non-uniform grids - Modify and add tests for new features
This pull request introduces support for custom grids in the
Geometryclass and updates thediscretize_spacefunction to handle such grids. It also includes extensive testing to ensure the new functionality works correctly. The most important changes are grouped as follows:Enhancements to
Geometryclass:gridparameter to theGeometryclass, allowing users to specify custom grids of breakpoints for each dimension. This parameter is stored as a private attribute_gridand exposed via a newgridproperty. (psydac/cad/geometry.py: [1] [2] [3] [4])Updates to
discretize_spacefunction:discretize_spacefunction to handle custom grids provided via thedomain_hobject. This includes safety checks for grid consistency, such as matching dimensions, grid length, sorting, and boundary alignment with domain limits. (psydac/api/discretization.py: psydac/api/discretization.pyR463-R493)Testing for custom grids:
gridparameter in various scenarios, including 1D, 2D, and 3D geometries, geometries with mappings, and non-uniform grids. Tests also verify that the grid is correctly stored and accessed. (psydac/cad/tests/test_geometry.py: [1] [2]Testing discretization with a custom grids:
psydac/api/tests/test_discretization_grid.py: [1]Exemple of discretization using a custom grid:
Example of custom grid in 2D and 3D: