-
Notifications
You must be signed in to change notification settings - Fork 67
Pickup dpg-dev and particleset/particlevector.hpp wrapping. #308
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: master
Are you sure you want to change the base?
Conversation
…can pass list/tuple
…r/SparseMatrix/DenseMatrix/DenseTensor returns numpy array via GetDataArray
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.
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) |
Copilot
AI
Jan 1, 2026
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.
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 |
Copilot
AI
Jan 1, 2026
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 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.
| if build_miniapps == '0': | ||
| libraries.append("mfem-common") |
Copilot
AI
Jan 1, 2026
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 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.
| mpiinc = '' | ||
|
|
||
| libraries = ['mfem',] | ||
| if build_miniapps == '0': |
Copilot
AI
Jan 1, 2026
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 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 |
Copilot
AI
Jan 1, 2026
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 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 |
Copilot
AI
Jan 1, 2026
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.
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 |
Copilot
AI
Jan 1, 2026
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.
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 |
Copilot
AI
Jan 1, 2026
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.
Import of 'dpg' is not used.
|
|
||
| try: | ||
| import mfem._par.dpg as dpg | ||
| except: |
Copilot
AI
Jan 1, 2026
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.
Except block directly handles BaseException.
|
|
||
| try: | ||
| import mfem._par.dpg as dpg | ||
| except: |
Copilot
AI
Jan 1, 2026
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.
'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.