Conversation
…can pass list/tuple
…r/SparseMatrix/DenseMatrix/DenseTensor returns numpy array via GetDataArray
There was a problem hiding this comment.
Pull request overview
This PR adds Python wrappers for MFEM's Discontinuous Petrov-Galerkin (DPG) miniapp utilities and includes a complete Python implementation of the parallel Maxwell DPG example. The key changes include:
- New Python DPG Maxwell example (pmaxwell.py) with support for various problem types (plane wave, Fichera oven, PML)
- Python wrappers for DPG utility classes (weakform, complexweakform, blockstaticcond, etc.)
- Wrappers for particleset and particlevector modules
- Fixes to GetDataArray methods to prevent premature object destruction
- Build system updates to enable miniapp compilation
- Support for non-square matrix coefficients
Reviewed changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| miniapps/dpg/pmaxwell.py | Complete Python implementation of parallel Maxwell DPG example with multiple problem types |
| miniapps/dpg/meshes/*.mesh | Mesh files for DPG examples (scatter.mesh, fichera-waveguide.mesh) |
| mfem/ser.py, mfem/par.py | Added imports for new dpg, particleset, particlevector modules |
| mfem/_ser/dpg.i, mfem/_par/dpg.i | SWIG interface files for DPG utilities wrapping |
| mfem/_ser/vector.i, mfem/_par/vector.i | Fixed GetDataArray to prevent premature destructor calls |
| mfem/_ser/densemat.i, mfem/_par/densemat.i | Fixed GetDataArray for DenseMatrix and DenseTensor |
| mfem/_ser/sparsemat.i, mfem/_par/sparsemat.i | Fixed GetDataArray, GetJArray, GetIArray for SparseMatrix |
| mfem/common/numba_coefficient*.* | Removed square matrix requirement for MatrixCoefficient |
| mfem/_ser/setup.py, mfem/_par/setup.py | Added build_miniapps configuration and library dependencies |
| _build_system/*.py | Build system updates for miniapp support |
| docs/changelog.txt | Documentation of changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| warning.warn("Rectangular matrix coefficient is experimental", UserWarning) | ||
| #if shape[0] != shape[1]: | ||
| # import warnings | ||
| # warnings.warn("Rectangular matrix coefficient is experimental", UserWarning) |
There was a problem hiding this comment.
Typo in variable name: "warning" should be "warnings". The code should use "warnings.warn(...)" instead of "warning.warn(...)".
| def load_module(module_name, module_code): | ||
| spec = importlib.util.spec_from_loader(module_name, loader=None, origin="dynamic") | ||
| module = types.ModuleType(module_name) | ||
| spec.loader.exec_module(module) if spec.loader else exec(module_code, module.__dict__) | ||
| sys.modules[module_name] = module | ||
| return module |
There was a problem hiding this comment.
The load_module function is defined but never used in this file. If this is intended for future use or as a utility, it should be documented. Otherwise, it should be removed to avoid dead code.
mfem/_ser/setup.py
Outdated
| if build_miniapps == '0': | ||
| libraries.append("mfem-common") |
There was a problem hiding this comment.
The logic here appears inverted. When build_miniapps == '0' (False), the code adds "mfem-common" to libraries. This seems backwards - shouldn't "mfem-common" be added when miniapps ARE being built (build_miniapps == '1')? Please verify this logic is correct.
mfem/_par/setup.py
Outdated
| mpiinc = '' | ||
|
|
||
| libraries = ['mfem',] | ||
| if build_miniapps == '0': |
There was a problem hiding this comment.
The logic here appears inverted. When build_miniapps == '0' (False), the code adds "mfem-common" to libraries. This seems backwards - shouldn't "mfem-common" be added when miniapps ARE being built (build_miniapps == '1')? Please verify this logic is correct.
| if build_miniapps == '0': | |
| if build_miniapps == '1': |
| command_obj.mfem_branch = '' | ||
| command_obj.mfem_debug = False | ||
| command_obj.mfem_build_miniapps = False | ||
| command_obj.mfem_miniapps = True |
There was a problem hiding this comment.
The default value for mfem_miniapps is changed from False to True. This is a significant behavior change that could affect existing workflows. Consider whether this should default to False for backward compatibility, or ensure this change is well-documented in the PR description.
| command_obj.mfem_miniapps = True | |
| command_obj.mfem_miniapps = False |
|
|
||
|
|
||
| try: | ||
| import mfem._par.dpg as dpg |
There was a problem hiding this comment.
Import of 'dpg' is not used.
| from numba import njit, void, int32, int64, float64, complex128, types | ||
| from mfem.common.bessel import yv as yn | ||
| from mfem.common.bessel import jv as jn | ||
| import os |
There was a problem hiding this comment.
Import of 'os' is not used.
| try: | ||
| import mfem._ser.complexweakform as complexweakform | ||
| import mfem._ser.commlexstaticcond as complexstaticcond | ||
| import mfem._ser.dpg as dpg |
There was a problem hiding this comment.
Import of 'dpg' is not used.
|
|
||
| try: | ||
| import mfem._par.dpg as dpg | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
|
|
||
| try: | ||
| import mfem._par.dpg as dpg | ||
| except: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
This PR includes
destructore is not pre-maturely called when return value is not stored to
a variable.