Operator refactor #42

Rombur · 2018-03-16T18:13:41Z

The first two commits relates to #37 and are mostly cosmetic. The third commit is more interesting. I have talked with @dalg24 and we thought that the current design was too complicated. This PR removes DealIIOperator (which was missing some functions and therefore was not an Operator as defined in concept.hpp) and it also removes DealIIMatrixOperator. Instead of specializing DealIIMatrixOperator, we now have classes that derived directly from Operator. The classes are DealIIMatrixOperator for the dealii::SparseMatrix and TrilinosMatrixOperator for the dealii::TrilinosWrappers::SparseMatrix. This classes are in the dealii_adapter namespace and should probably be moved in a dealii subdirectory later. The new structure is simpler and allows us to use ETI.

codecov-io · 2018-03-16T18:15:46Z

Codecov Report

Merging #42 into master will decrease coverage by 0.77%.
The diff coverage is 71.21%.

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
- Coverage      89%   88.23%   -0.78%     
==========================================
  Files          16       19       +3     
  Lines        1110     1113       +3     
==========================================
- Hits          988      982       -6     
- Misses        122      131       +9

Impacted Files	Coverage Δ
tests/test_eigenvectors.cc	`100% <ø> (ø)`	⬆️
tests/test_hierarchy.cc	`82.23% <ø> (-0.24%)`	⬇️
source/amge_host.cc	`100% <ø> (ø)`	⬆️
include/mfmg/amge_host.templates.hpp	`100% <ø> (ø)`	⬆️
tests/test_agglomerate.cc	`100% <ø> (ø)`	⬆️
include/mfmg/amge_host.hpp	`100% <ø> (ø)`	⬆️
tests/test_restriction_matrix.cc	`100% <ø> (ø)`	⬆️
include/mfmg/concepts.hpp	`100% <100%> (ø)`
include/mfmg/dealii_adapters.hpp	`90.9% <100%> (ø)`
include/mfmg/hierarchy.hpp	`97.64% <100%> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 192549b...c41f331. Read the comment docs.

aprokop

Overall, I am pretty happy with the approach. I am failing to remember why the original DealIIOperator did not inherit from Operator concept. Inheriting certainly simplifies things.

The only serious issue I have is that the Operator concept should be split to the original Operator and MatrixOperator concept, and make DealIIMatrixOperator and TrilinosMatrixOperator inherit from later, while SmootherOperator and DirectOperator inherit from former.

aprokop · 2018-03-16T23:46:15Z

include/mfmg/concepts.hpp

+  virtual size_t m() const = 0;
+  virtual size_t n() const = 0;
+  virtual void apply(vector_type const &x, vector_type &y) const = 0;
+  virtual std::shared_ptr<operator_type> transpose() const = 0;


The concept of operator should really only have the meaning of how to produce right hand side by operating on left hand side. Its interface should clearly indicate that. Thus, I believe that member functions like transpose() and multiply() do not belong in it, as they somewhat restrict the concept to operators corresponding to matrices. It also becomes a rabbit hole if we start creating operators with more required functions and putting them in Operator. Therefore, I'd like to retain the old interface with apply and build_{range,domain}_vector. We could even take m() and n() out but that would most likely require Vector concept.

What I would propose to do instead is to create a concept of MatrixOperator which inherits Operator. Then, in DealIIMatrixOperator and TrilinosMatrixOperator could inherit from that instead.

Yeah, good idea.

aprokop · 2018-03-16T23:47:31Z

include/mfmg/dealii_operator.hpp

-    dealii::SparseMatrix<typename VectorType::value_type>, VectorType>
-    : public DealIIOperator<VectorType>
+template <typename VectorType>
+class DealIIMatrixOperator : public Operator<VectorType>


As I mentioned above, this should inherit from MatrixOperator concept.

aprokop · 2018-03-16T23:48:23Z

include/mfmg/dealii_operator.hpp

-                           dealii::TrilinosWrappers::SparseMatrix, VectorType>
-    : public DealIIOperator<VectorType>
+template <typename VectorType>
+class TrilinosMatrixOperator : public Operator<VectorType>


I propose either renaming TrilinosMatrixOperator to DealIITrilinosMatrixOperator or dropping DealII from DealIIMatrixOperator for consistency. They both live in some dealii namespace so that should be fine. Judging of how this patch renames DealIISmootherOperator to SmootherOperator, I am fine changing DealIIMatrixOperator to simply MatrixOperator.

But then how do you call the matrix operator that derives from Operator? I have used TrilinosMatrixOperator and DealIIMatrixOperator because when Damien saw DealIIMatrixOperator, he assumed that we had sth called TrilinosMatrixOperator

Yeah, while we could differentiate by namespaces, I like calling the concept of matrix operator MatrixOperator. The only minor issue I have right now is that TrilinosMatrixOperator does not indicate that it is deal.ii specific. I guess the namespace does that, but that's a bit of extra effort. I could live with leaving it as it is, but would prefer DealIITrilinosMatrixOperator and then probably DealIIDirectOperator with DealIISmootherOperator. In any case, this all is hidden from Hierarchy.

aprokop · 2018-03-16T23:50:45Z

include/mfmg/dealii_operator.hpp

-    }
-  }
+  virtual std::shared_ptr<operator_type>
+  multiply(operator_type const &operator_b) const override final;


These functions really don't belong to smoother, and should be gone once MatrixOperator is moved out of Operator concept.

aprokop · 2018-03-16T23:54:23Z

include/mfmg/dealii_operator.templates.hpp

+std::shared_ptr<VectorType>
+DealIIMatrixOperator<VectorType>::build_domain_vector() const
+{
+  ASSERT_THROW_NOT_IMPLEMENTED();


build_domain_vector and build_range_vector should probably be implemented for DealIIMatrixOperator. I guess they currently are not only because we don't use them.

Yes, I think it's fine for now not having them

aprokop · 2018-03-16T23:57:08Z

include/mfmg/dealii_operator.hpp

+  void initialize(std::string const &prec_type);
+
+  matrix_type const &_matrix;
+  std::unique_ptr<dealii::TrilinosWrappers::PreconditionBase> _smoother;


OK, that was me being stupid, I did not find Trilinos' PreconditionerBase in deal.ii.

aprokop · 2018-03-16T23:59:33Z

include/mfmg/dealii_operator.hpp

-  dealii::TrilinosWrappers::PreconditionILU _ilu_smoother;
+  void initialize(std::string const &prec_type);
+
+  matrix_type const &_matrix;


We need to add a comment here. Something like this:
Currently, dealii preconditioners do not support a mode to update a given vector, instead replacing it. For smoothers, this mode is necessary to operator. We workaround this issue by doing extra computations in "apply", however this requires residual computation. Therefore, we need to store a reference to matrix.

Get ride of DealIIOperator and DealIIMatrixOperator. Instead of using partial template specialization to use different types of matrix, we derive different classes from Operator. This simplifies the structure and allows for ETI.

ghost assigned Rombur Mar 16, 2018

ghost added the in progress label Mar 16, 2018

aprokop requested changes Mar 17, 2018

View reviewed changes

Rombur added 5 commits March 23, 2018 10:28

Rename adapters_dealii.hpp to dealii_adapters.hpp

d47f0bc

Use typename instead of class and move const to the right

36a077e

Refactor dealii_operator

4b160c0

Get ride of DealIIOperator and DealIIMatrixOperator. Instead of using partial template specialization to use different types of matrix, we derive different classes from Operator. This simplifies the structure and allows for ETI.

Introduce MatrixOperator derived from Operator

adb6a4a

Fix a few warnings

c41f331

Rombur force-pushed the cuda_hierarchy branch from 99451af to c41f331 Compare March 23, 2018 14:29

Rombur closed this Mar 23, 2018

Rombur reopened this Mar 23, 2018

ghost added in progress and removed in progress labels Mar 23, 2018

Rombur closed this Mar 23, 2018

Rombur reopened this Mar 23, 2018

ghost added in progress and removed in progress labels Mar 23, 2018

aprokop approved these changes Mar 23, 2018

View reviewed changes

aprokop merged commit 03f28e3 into ORNL-CEES:master Mar 23, 2018

ghost removed the in progress label Mar 23, 2018

Rombur deleted the cuda_hierarchy branch March 26, 2018 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operator refactor #42

Operator refactor #42

Rombur commented Mar 16, 2018

codecov-io commented Mar 16, 2018 •

edited

Loading

aprokop left a comment

aprokop Mar 16, 2018

aprokop Mar 16, 2018

Rombur Mar 18, 2018

aprokop Mar 16, 2018

aprokop Mar 16, 2018

Rombur Mar 18, 2018

aprokop Mar 18, 2018

aprokop Mar 16, 2018

aprokop Mar 16, 2018

Rombur Mar 18, 2018

aprokop Mar 16, 2018 •

edited

Loading

aprokop Mar 16, 2018 •

edited

Loading

Operator refactor #42

Operator refactor #42

Conversation

Rombur commented Mar 16, 2018

codecov-io commented Mar 16, 2018 • edited Loading

Codecov Report

aprokop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprokop Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

aprokop Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Mar 16, 2018 •

edited

Loading

aprokop Mar 16, 2018 •

edited

Loading

aprokop Mar 16, 2018 •

edited

Loading