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

Dsl/implicit operators #246

Merged
merged 6 commits into from
Feb 1, 2025

Conversation

HenningScheufler
Copy link
Collaborator

@HenningScheufler HenningScheufler commented Jan 25, 2025

Motivation

The DSL needs to handle implicit operators.

This PR handles strategy how to assemble the linear system and proposes architectural changes to the current design

Assumption

  • the sparsity pattern is identical for all operators in the DSL

Consequences

  • the linear system needs an identifier, which sparsity pattern is used --> currently a std::string (for now)
  • the DSL is heavily linked to the cell-centered finite volume method
  • the implicit source terms and temporal operators use the same sparsity pattern, despite being diagonal matrices

Tradeoffs

A long-term goal of NeoFOAM is to build a multi physic solver, the current assumption is that every region as its own physics with its on strategy to solve PDE's. The coupling between the regions would be via the boundary conditions.

Pro

  • simpler design
  • simpler implementation of the operators and DSL as the data structures are know
  • increased performance: operators and expression know which numerical method they support and can optimize the performance (e.g. fvcc --> flux-based summation and other strategies to compute the operators depending scaling factors)

Contra

  • Abstraction needed to accumulate the different regions into one coupled system

Recommendation

Currently, only one numerical method is supported (fvcc) and the coupling between the methods, will be tackled later. v0.4+. Defining a general abstraction, how the DSL works for every numerical method is complex and is a tradeoff between universality and performance.

A DSL should be more tightly coupled to a specific method --> e.g. move dsl folder into finite volume

A general abstraction, how the different linear system and rhs should be coupled over the regions seems still feasible as it only requires the concatenation of each linear system. However, this should be tackled if other PDE solution strategies are implemented

@HenningScheufler HenningScheufler added enhancement New feature or request discussion labels Jan 25, 2025
@HenningScheufler HenningScheufler added this to the 0.2 Release milestone Jan 25, 2025
@HenningScheufler
Copy link
Collaborator Author

HenningScheufler commented Jan 25, 2025

@greole @bevanwsjones @MarcelKoch

Please write your general comments into the issue #248

Code review should still be done in the PR. This makes it easier to track the required substeps. Sadly, PR cannot be added as subissues

Note: I will summaries the decision in the PR description

Copy link

Deployed test documentation to https://exasim-project.com/NeoFOAM/Build_PR_246

Copy link
Contributor

@greole greole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some quick comments

#include "NeoFOAM/dsl/operator.hpp"
#include "NeoFOAM/core/error.hpp"

namespace la = NeoFOAM::la;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think that isn't even necessary inside the NeoFOAM namespace you should be able to use just la::

LinearSystem(
const CSRMatrix<ValueType, IndexType>& matrix,
const Field<ValueType>& rhs,
const std::string& sparsityPattern
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is sparsityPattern a string?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a enum is better?

@HenningScheufler HenningScheufler changed the base branch from main to stack/implicitOperators February 1, 2025 08:51
@HenningScheufler
Copy link
Collaborator Author

This will be merged in the stack/implicitOperator branch improvements can be added by seperate PR into stack/implicitOperator

@HenningScheufler HenningScheufler merged commit 5e8556c into stack/implicitOperators Feb 1, 2025
9 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

2 participants