Skip to content
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

Merge MnaSolver and MnaSolverDirect classes #307

Open
MarvinTollnitschRWTH opened this issue Jun 18, 2024 · 1 comment
Open

Merge MnaSolver and MnaSolverDirect classes #307

MarvinTollnitschRWTH opened this issue Jun 18, 2024 · 1 comment
Assignees
Labels
help wanted Extra attention is needed refactor Issues related to code refactoring

Comments

@MarvinTollnitschRWTH
Copy link
Contributor

MarvinTollnitschRWTH commented Jun 18, 2024

Current implementation

Broadly speaking, the MnaSolver class sets up system related objects (MNA-components, nodes, vectors, matrices, etc.) while the MnaSolverDirect class deals with the task system, the solving methods and introduces the adapters derived from the DirectLinearSolver class. The latter are used to actually solve linear equation systems given in matrix-vector format. Stamping is split between both classes, with rightSideVector-stamping being located in the MnaSolver and (switched) system matrix stamping located in the MnaSolverDirect class.

Proposed Changes

The question is if this separation into two solver classes is sensible. Equation system, component and task setup can be done in the same class as well as stamping and the solving for which the Linear Adapter member can be moved to the same class as well. This way, any confusion regarding two similar MNA solvers as well as the "Direct"-suffix can be mitigated. Also, all functionality (members, methods) needed for the MNA approach can be grouped into one common place. One disadvantage could be the increased size of the resulting solver class with both current solver classes being rather large already.

Future plans

If the approach of merging the solvers is followed, more MNA related solvers could follow in the future. As of now, two solving methods within the MNA approach are already implemented within the MnaSolverDirect class: solve() used by the SolveTask and solveWithMatrixRecomputation() used by the SolveTaskRecomp. With the task system already used this way, it might be sensible to move the functionality of the DiakopticsSolver and potential iterative MNA solvers for nonlinear systems into this one combined solver class as well; it would only require adding the corresponding tasks, solve methods and data attributes (subnets for Diakoptics, for example). This way, all variants of the same underlying MNA solving approach can be aggregated into one solver class.

Advantages/Disadvantages

✔️one class for all MNA related setup processes, attributes and solving methods
✔️task system allows for simple expansion of merged MNA solver class
✔️ no confusion over similar MnaSolver and MnaSolverDirect
✔️ cleaner (smaller) solver hierarchy

❌ very large merged MNA solver class

Further details

A branch following the merged MNA solver approach can be found here.

Current Solver structure:

DPsim_MnaSolvers_current

Solver structure after solver merge:

DPsim_MnaSolvers_merged

@georgii-tishenin georgii-tishenin added the help wanted Extra attention is needed label Jun 18, 2024
@georgii-tishenin georgii-tishenin added the refactor Issues related to code refactoring label Jun 26, 2024
@georgii-tishenin
Copy link
Collaborator

Hi team @dinkelbachjan, @leonardocarreras, @m-mirz, @martinmoraga, @n-eiling, @pipeacosta, @stv0g,

@MarvinTollnitschRWTH has been working on merging MNASolver and MNASolverDirect. So far, the discussion was between @gnakti, Marvin and me.

We would like your feedback on:

  • The feasibility of this idea

  • Key considerations to keep in mind

  • Whether we can proceed

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed refactor Issues related to code refactoring
Projects
None yet
Development

No branches or pull requests

2 participants