Add radial DG discretization#609
Conversation
|
Hi Yuvj, now that you have already implemented rLRMP and rGRM as separate units, I think you can first (EOC) test this in pyhton before refactoring into the |
2470a36 to
756efad
Compare
eb9c111 to
fe88c77
Compare
d0c96f7 to
03f4cb7
Compare
|
CADET-Verification EOC tests for radial DG: Yuvraj2710/CADET-Verification@radialDG-verification Tests radial DG convergence (polyDeg 2-5) with constant and variable dispersion (RADIAL_POWER_LAW) using the CADET-Verification framework. All polynomial degrees achieve expected convergence order. |
|
Hi @jbreue16, I suppose this impl. of DG radial flow is working, and is ready for a review. I'll remove the rLRMP and rGRM which are as separate units currently, but there's also with |
jbreue16
left a comment
There was a problem hiding this comment.
Hi Yuvj, can you show me the convergence results and are they using a FV reference?
EOC Convergence Verification — Radial DG Bulk TransportRan EOC tests for radial DG (bulk transport only, rLRM no binding) using the CADET-Verification benchmark parameters ( Time integrator tolerances: CADET-Core DG vs FV WENO3 reference (Z=262,144): P1 (expected order 2):
P2 (expected order 3):
P3 (expected order 4):
P4 (expected order 5):
P5 (expected order 6):
|
|
Two things that are weird here: EOC is unexpectedly high sometimes, e.g. 8.34 for P4, and convergence stops at around 1e-8. But the most relevant issue is convergence stopping at 1e-8. Can you set time integration tolerances |
|
Yes I did took tolerances to |
|
radCOL1D_transport_1comp_WENO3_benchmark1_FV_Z262144.zip I have attached a reference solution with at least 7e-11 accuracy (262.144 cells) which you can use as reference. You can fix cadet/CADET-Verification#110 first and then compute the EOC for DG again |
Just updated the EOC results |
jbreue16
left a comment
There was a problem hiding this comment.
Im very happy to see the great progress here.
Just reviewing this large PR on GitHub wasn't enough so I went into the code and addressed some issues myself. This PR needs a lot of clean-up and some fixes and needs to be rebased, force pushed. Thus, I made a backup branch: https://github.com/cadet/CADET-Core/tree/back_up/DGRadialFeature
Please read through the changes I made below and tell me if you disagree with any of this
- The commits were not separated cleanly, there was stuff all over the place. I think its best to create two big PR's out of this: 1) radial DG incorporated to
ColumnModel1DandLumpedRateModelWithoutPoresDG, I think this can be merged fast. 2) adding variable coefficients, Ive seen some implementation issues here so that probably needs further refinement and we shouldn't wait with mergeing the rest, hence a separate PR. - I removed some unwanted changes like increase of default AD directions,
PAR_DISC_TYPEwas made optional. I suppose there was no clear intent behind those changes and they are artifacts from the long list of changes made. - I updated how the integration mode is handled, making this part more modular for the seamless use of another convDispOp: #644
- #646 fix of input group for filmDiff parDep in LRMP_FV as independent PR, added fixed docs and test for surfDiff
28987bc to
890f9cc
Compare
This comment was marked as spam.
This comment was marked as spam.
d7b7177 to
a8504a1
Compare
a8504a1 to
01ee54d
Compare
|
Hi @Yuvraj2710 please don't force push with squashes unless we agree to clean things up, such froce-pushes once I've started reviewing make it impossible for me to review. The final squash into one commit will happen at the very end, probably through the GitHub interface, not from a force push. Ive kept the changes you force pushed in 5bb4f16 and 01ee54d, except for 1) the changes made to the ConvDispOp, which reintroduced an ambiguous interface that I removed and 2) the changes which falsely incorporated the rLRM into the |
9ba0937 to
d9ac843
Compare
96ccbb1 to
8e9f566
Compare
8e9f566 to
d2186e9
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an arbitrary-order discontinuous Galerkin (DG) spatial discretization for CADET’s radial-flow column models by introducing a dedicated radial DG convection–dispersion operator, wiring it into radial unit operations, and extending tests and documentation to cover the new discretization.
Changes:
- Implement radial DG transport support (including rho-weighted mass matrices and variable-coefficient handling) and expose it through the existing DG operator framework.
- Generalize
ColumnModel1DandLumpedRateModelWithoutPoresDGto support axial vs radial DG via templated convection–dispersion operators, and update model selection/registration accordingly. - Add radial DG regression/reference tests, helper utilities, and JSON benchmark models; update radial-flow documentation to include DG configuration options.
Reviewed changes
Copilot reviewed 29 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/RadialLumpedRateModelWithPores.cpp | Adds extensive radial LRMP_DG CI test coverage (benchmarks, Jacobians, sensitivity, CI comparisons vs FV references). |
| test/RadialLumpedRateModelWithoutPores.cpp | Adds radial LRM_DG CI test coverage analogous to existing FV tests plus DG-vs-FV reference checks. |
| test/RadialGeneralRateModel.cpp | Adds radial GRM_DG CI tests, including parameter-dependence Jacobian checks and DG-vs-FV reference benchmark. |
| test/ParticleHelper.hpp | Extends arrowhead Jacobian helper API to accept a spatial method argument. |
| test/ParticleHelper.cpp | Implements the new spatial-method-aware arrowhead Jacobian helper. |
| test/JsonTestModels.cpp | Updates JSON model factories to correctly detect GRM/LRMP behavior for radial unit type strings and fixes frustum/radial parameter placement. |
| test/data/model_radLRMP_dynLin_1comp_sensbenchmark1.json | Updates LRMP benchmark model to use DG discretization settings. |
| test/data/model_radLRMP_DG_noBnd_1comp_eocbenchmark.json | Adds new DG model for EOC/benchmark comparisons (no binding). |
| test/data/model_radLRMP_DG_linBnd_1comp_eocbenchmark.json | Adds new DG model for EOC/benchmark comparisons (linear binding). |
| test/data/model_radLRM_dynLin_1comp_sensbenchmark1.json | Updates LRM benchmark model to use DG discretization settings. |
| test/data/model_radLRM_DG_noBnd_1comp_eocbenchmark.json | Adds new radial LRM DG model for EOC/benchmark comparisons (no binding). |
| test/data/model_radLRM_DG_linBnd_1comp_eocbenchmark.json | Adds new radial LRM DG model for EOC/benchmark comparisons (linear binding). |
| test/data/model_radGRM_dynLin_1comp_sensbenchmark1.json | Refactors GRM benchmark JSON structure and preserves FV settings while aligning with updated unit-type detection. |
| test/data/model_radGRM_DG_noBnd_1comp_eocbenchmark.json | Adds new radial GRM DG model for EOC/benchmark comparisons (no binding). |
| test/ColumnTests.hpp | Declares new radial DG benchmark/convergence helper test APIs. |
| test/ColumnTests.cpp | Implements radial DG vs FV reference comparison and self-convergence EOC helper. |
| test/ColumnModel1D.cpp | Adds radial ColumnModel1D DG test matrix (Jacobian, sensitivities, particle-type coverage, variable coefficient checks). |
| src/libcadet/model/parts/DGToolbox.hpp | Adds quadrature/node utilities (Chebyshev + Gauss) and radial DG helper declarations (weighted mass, radial dispersion matrix, basis derivative). |
| src/libcadet/model/parts/DGToolbox.cpp | Implements new quadrature/node utilities and radial dispersion matrix integration helper. |
| src/libcadet/model/parts/ConvectionDispersionOperatorDG.hpp | Introduces RadialConvectionDispersionOperatorBaseDG with radial DG residual/Jacobian scaffolding and flux handling. |
| src/libcadet/model/LumpedRateModelWithoutPoresDG.hpp | Templates the DG LRM unit on the convection–dispersion operator and adds a radial DG identifier. |
| src/libcadet/model/LumpedRateModelWithoutPoresDG.cpp | Refactors implementation to template form and explicitly instantiates axial + radial DG variants. |
| src/libcadet/model/ColumnModelBuilder.cpp | Enables radial DG unit selection and registers axial/radial DG models under appropriate identifiers. |
| src/libcadet/model/ColumnModel1D.hpp | Templates ColumnModel1D on the convection–dispersion operator, adds radial identifier mapping, and adds explicit extern template declarations. |
| src/libcadet/model/ColumnModel1D.cpp | Refactors ColumnModel1D implementation to template form and explicitly instantiates axial + radial DG variants. |
| src/libcadet/model/ColumnModel1D-LinearSolver.cpp | Updates linear solver methods to template form for the generalized ColumnModel1D. |
| src/libcadet/model/ColumnModel1D-InitialConditions.cpp | Updates initial-condition and sensitivity initialization code to template form for the generalized ColumnModel1D. |
| doc/interface/unit_operations/radial_flow_column_1D_config.rst | Documents DG as an available radial discretization method and adds DG-specific parameters and notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::vector<StateType> c_star(_nElem + 1); | ||
| std::vector<StateType> g_star(_nElem + 1); | ||
| std::vector<double> d_rad_i(_nElem + 1); | ||
|
|
||
| computeNumericalFluxesRadial<StateType, ParamType>(yBulk, d_rad, c_star, g_star, d_rad_i); |
| * b. Scale g by (2/Δρ) | ||
| * c. Compute numerical fluxes c*, g* | ||
| * d. Compute main equation Dc via surface + volume integrals |
| refValues.push_back(ref_outlet[i]); | ||
| } | ||
| } | ||
|
|
- Add RadialLumpedRateModelWithoutPoresDG, RadialLumpedRateModelWithPoresDG, RadialGeneralRateModelDG with full residual, analytic Jacobian, and parameter sensitivity support - Extend ConvectionDispersionOperatorDG and DGToolbox for radial geometry - Register radial DG models in ColumnModelBuilder - Add EOC convergence tests for rLRM DG polyDeg 1-5 (all pass) - Add testRadialDGConvergence helper and reference benchmark infrastructure Co-authored-by: Jan Breuer <74359367+jbreue16@users.noreply.github.com>
27ef6af to
0113e1d
Compare
This PR adds an arbitrary order DG discretization for the radial flow models
Todo
ColumnModel1Dunit