search

ikarus-project / ikarus

Ikarus is a C++-based library that uses DUNE modules to solve partial differential equations with the finite element method and more.
https://ikarus-project.github.io/
Other
4 stars 2 forks source link

Refactor nonlinear operator creation for finite elements #289

Closed rath3t closed 3 weeks ago

rath3t commented 2 months ago

This PR also fixes #290 .

rath3t commented 2 months ago

Basically the C++ side is finished. Im working now on the Python side but the crucial thing is the name of the new class FEProblem. I really dont like the name, dou you have any ideas?

rath3t commented 2 months ago

Also I dont like QuantityType as a name for the objects for the request.

Do you think we should als change the member functions of the assembler to have one name getMatrix(...,QuantityType::Full) and get rid of the different names such as getReducedMatrix

tarun-mitruka commented 2 months ago

Also I dont like QuantityType as a name for the objects for the request.

Do you think we should als change the member functions of the assembler to have one name getMatrix(...,QuantityType::Full) and get rid of the different names such as getReducedMatrix

Since QuantityType actually signifies how the homogeneous Dirichlet boundary conditions are enforced, may be we can name it in a similar fashion. What do you then think about the following suggestions?

  1. EnforceBCType or EnforceDirichletBCType
  2. ApplyBCType or ApplyDirichletBCType
  3. AssemblerOptions
  4. BCOptions
  5. EnforcingBCOptions and similar...

We can also have non-type template bool that says if the BC condition is to be enforced and default it to true. We can then also get rid of QuantityType::Raw and just return via getRawMatrix if this bool is set to false. I am okay with having the member function of the assembler to one name like getMatrix or even get rid of "get" and use just matrix, if possible.. I am also open for this change to happen in a separate PR.

rath3t commented 2 months ago

Ok I though about it back and forth. I I think the cleanest option is to merge FEProblem with the assemblers.

This entangles things more but I want to go that way. Think about the discussion I told you to read in Bob Martin "Clean code". In "John K. Ousterhout. A Philosophy of Software Design" the authors argues that classes should be deep and not shallow. In my point of view right now this is the way to go to make the assembler class deeper. Instead of seperating tiny things in several classes as more or less advertised by Bob Martin "Clean code".

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 86.36364% with 33 lines in your changes missing coverage. Please review.

Project coverage is 91.23%. Comparing base (bc987df) to head (0ef6bea).

Files Patch % Lines
ikarus/finiteelements/ferequirements.hh 0.00% 18 Missing :warning:
ikarus/assembler/simpleassemblers.hh 88.46% 3 Missing :warning:
...rus/finiteelements/mechanics/kirchhoffloveshell.hh 70.00% 3 Missing :warning:
ikarus/solver/nonlinearsolver/trustregion.hh 88.00% 3 Missing :warning:
ikarus/utils/nonlinopfactory.hh 88.46% 3 Missing :warning:
...s/solver/nonlinearsolver/nonlinearsolverfactory.hh 83.33% 2 Missing :warning:
ikarus/finiteelements/fehelper.hh 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #289 +/- ## ========================================== + Coverage 85.27% 91.23% +5.95% ========================================== Files 60 63 +3 Lines 1983 2053 +70 ========================================== + Hits 1691 1873 +182 + Misses 292 180 -112 ``` | [Flag](https://app.codecov.io/gh/ikarus-project/ikarus/pull/289/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ikarus-project) | Coverage Δ | | |---|---|---| | [tests](https://app.codecov.io/gh/ikarus-project/ikarus/pull/289/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ikarus-project) | `91.23% <86.36%> (+5.95%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ikarus-project#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rath3t commented 3 weeks ago

Ok I will hit autmerge here, even if the example docs are outdated now.