Refactor CSRMatrix #359

angriman · 2019-06-27T11:38:47Z

This adds a template parameter to the CSRMatrix class allowing to store generic data types. The class is renamed to CSRGeneralMatrix. Additional minor changes:

Remove ad-hoc implementation of binary search in favor of std::lower_bound
Add noexcept operator where applicable
Replace push_back with emplace_back where applicable (and addition of two new constructors for Triplet)

include/networkit/algebraic/CSRGeneralMatrix.hpp

include/networkit/algebraic/CSRMatrix.hpp

include/networkit/algebraic/CSRGeneralMatrix.hpp

avdgrinten

LGTM now. I did not review the algorithm; I assume that they did not change except for changing double -> ValueType (?).

include/networkit/algebraic/CSRGeneralMatrix.hpp

avdgrinten · 2019-06-30T14:50:54Z

Why does the code break with an earlier version of Clang? We should not increase the version of Clang without a good reason. People who want run NetworKit on clusters that they do not control might not have the option to upgrade Clang.

angriman · 2019-06-30T15:56:43Z

The build fails with clang 3.8 because of this error:

/home/travis/build/networkit/networkit/include/networkit/algebraic/CSRGeneralMatrix.hpp:1231:10: error: 
      definition with same mangled name as another definition
    void forNonZeroElementsInRow(index row, L handle) const {
         ^
/home/travis/build/networkit/networkit/include/networkit/algebraic/CSRGeneralMatrix.hpp:1231:10: note: 
      previous definition is here

I searched for this error and did not find much apart this page. It could be a bug in clang 3.8 but I am not sure about that. Do you have some other solutions in mind?

angriman · 2019-07-04T12:12:35Z

Moving forNonZeroElementsInRow somewhere else in the class did not solve the issue, I still get the same error. Shall we switch to clang 3.9?

angriman · 2021-03-10T10:06:35Z

Now that we raised the minimum supported clang version to 3.9 this PR can be reviewed.

fabratu

LGTM.

angriman · 2021-04-19T13:18:39Z

We also need approval from @avdgrinten.

avdgrinten

I have one comment below.

As a side note: a few algorithms are really inefficient here, for example setValue(). There is no way to implement setValue() well for CSR, so why does the class provide it at all?

include/networkit/algebraic/CSRGeneralMatrix.hpp

angriman · 2021-04-20T20:19:47Z

I agree that there are several inefficiencies in this class that should be fixed, I already fixed a trivial one in the last commit. However, I think that fixing those inefficiencies is out of the scope of this PR and it should be done on a separate PR.

hmeyerhenke · 2021-04-21T07:38:05Z

Not sure that setValue() is really obsolete. But the docs should be clear about the inherent inefficiency. Other -- unnecessary -- inefficiencies should be addressed. This is rather old code that has not been touched for quite some time, I believe.

hmeyerhenke · 2021-04-21T07:38:59Z

But I leave it up to you guys if they are addressed in this PR or separately.

avdgrinten · 2021-04-21T08:12:40Z

It's not necessarily the case that the code is inefficient due to the implementation but it just provides tons of operations that you really would not use if you want your code to be fast (setting values, getting individual values, extracting columns, etc.). These operations are simply not fast because CSR cannot implement them in a fast way.

hmeyerhenke · 2021-04-21T08:15:42Z

I know @avdgrinten. Such operations should not be the preferred way to do things, I completely agree. But there may be cases where they are simply necessary. We can make a decision, though, that we do not want to support such cases. But that's a decision I'd like to be involved in and should follow from a comprehensive discussion of use cases.

angriman · 2021-04-21T08:17:44Z

It might be a good idea to open a new issue/discussion about that.

angriman · 2021-04-22T13:20:36Z

Are there any other issues that need to be addressed in this PR?

avdgrinten · 2021-05-28T11:24:08Z

include/networkit/algebraic/AlgebraicGlobals.hpp

+
+    Triplet() = default;
+
+    Triplet(index row, index column, double value) : row(row), column(column), value(value) {}


Why is this change done?

avdgrinten · 2021-05-28T11:32:28Z

include/networkit/algebraic/CSRGeneralMatrix.hpp

+    /** Default destructor */
+    virtual ~CSRGeneralMatrix() = default;


Why is this destructor virtual? Does the class have any virtual methods?

avdgrinten · 2021-05-28T11:37:24Z

include/networkit/algebraic/CSRGeneralMatrix.hpp

+            }
+        }
+
+#ifndef NDEBUG


This should be gated behind the sanity check macros, right?. Seems to be an expensive check.

avdgrinten · 2021-06-14T11:28:56Z

include/networkit/algebraic/CSRGeneralMatrix.hpp

+    /**
+     * @return Number of rows.
+     */
+    inline count numberOfRows() const noexcept { return nRows; }


inline is redundant if a method is defined within class { ... } or struct { ... }.

avdgrinten · 2021-06-14T11:34:25Z

include/networkit/algebraic/Vector.hpp

-                triplets.push_back({i, j, result});
+                triplets.emplace_back(i, j, result);


Unrelated change?

If I do not change this then the clang-tidy build fails due to modernize-use-emplace.

Why doesn't clang-tidy fail right now?

avdgrinten · 2021-06-14T11:34:44Z

include/networkit/numerics/LAMG/MultiLevelSetup.hpp

-                triples.push_back({k, coarseIndex[j], w});
+                triples.emplace_back(k, coarseIndex[j], w);


Unrelated change?

avdgrinten · 2021-06-14T11:34:50Z

networkit/cpp/algebraic/DynamicMatrix.cpp

-                    triplets.push_back({i, j, value});
+                    triplets.emplace_back(i, j, value);


Unrelated change?

avdgrinten · 2021-06-14T11:34:55Z

networkit/cpp/io/MatrixMarketReader.cpp

-            triplets.push_back({i,j,val});
+            triplets.emplace_back(i,j,val);


Unrelated change?

avdgrinten · 2021-06-14T11:35:01Z

networkit/cpp/numerics/LAMG/MultiLevelSetup.cpp

-            triplets.push_back({i,j,value});
+            triplets.emplace_back(i,j,value);


Unrelated change?

avdgrinten · 2021-06-14T11:35:09Z

networkit/cpp/viz/PivotMDS.cpp

-            triplets.push_back({v, j, dist[v]});
+            triplets.emplace_back(v, j, dist[v]);


Unrelated change?

angriman · 2021-06-17T09:02:57Z

I addressed the comments, the emplace_back were not really necessary, so I removed them.

avdgrinten

LGTM now.

avdgrinten requested changes Jun 27, 2019

View reviewed changes

include/networkit/algebraic/CSRGeneralMatrix.hpp Outdated Show resolved Hide resolved

include/networkit/algebraic/CSRMatrix.hpp Show resolved Hide resolved

angriman force-pushed the refactor/CSRMatrix branch from cd5ad55 to d573b43 Compare June 27, 2019 22:20

avdgrinten requested changes Jun 28, 2019

View reviewed changes

include/networkit/algebraic/CSRGeneralMatrix.hpp Outdated Show resolved Hide resolved

include/networkit/algebraic/CSRGeneralMatrix.hpp Outdated Show resolved Hide resolved

angriman force-pushed the refactor/CSRMatrix branch 2 times, most recently from b45faa6 to db7ba88 Compare June 28, 2019 14:10

avdgrinten approved these changes Jun 28, 2019

View reviewed changes

avdgrinten requested changes Jun 28, 2019

View reviewed changes

include/networkit/algebraic/CSRGeneralMatrix.hpp Outdated Show resolved Hide resolved

avdgrinten self-requested a review June 28, 2019 14:52

angriman force-pushed the refactor/CSRMatrix branch 4 times, most recently from f11c992 to 8be5f46 Compare June 30, 2019 10:16

angriman force-pushed the refactor/CSRMatrix branch 2 times, most recently from 7421696 to 1e247b0 Compare July 13, 2019 22:38

angriman mentioned this pull request Mar 4, 2020

Compile numerics #516

Merged

fabratu changed the base branch from Dev to master May 29, 2020 16:54

angriman force-pushed the refactor/CSRMatrix branch from 1e247b0 to 69ae7bc Compare January 5, 2021 13:24

fabratu mentioned this pull request Feb 17, 2021

Raises minimum requirement for clang 3.8 -> 3.9 #709

Merged

angriman force-pushed the refactor/CSRMatrix branch 2 times, most recently from dca2088 to ede57c6 Compare March 9, 2021 16:13

angriman force-pushed the refactor/CSRMatrix branch 2 times, most recently from 10efc2b to 89744e7 Compare April 19, 2021 07:48

fabratu previously approved these changes Apr 19, 2021

View reviewed changes

avdgrinten reviewed Apr 20, 2021

View reviewed changes

include/networkit/algebraic/CSRGeneralMatrix.hpp Show resolved Hide resolved

angriman dismissed fabratu’s stale review via da33d2a May 25, 2021 12:27

angriman force-pushed the refactor/CSRMatrix branch 2 times, most recently from da33d2a to 07b16b6 Compare May 31, 2021 14:00

avdgrinten reviewed Jun 14, 2021

View reviewed changes

angriman force-pushed the refactor/CSRMatrix branch 3 times, most recently from 26e5a2b to b5cd845 Compare June 17, 2021 08:37

avdgrinten self-assigned this Jun 23, 2021

angriman force-pushed the refactor/CSRMatrix branch from b5cd845 to ca9dd75 Compare June 23, 2021 13:12

angriman added 2 commits June 23, 2021 15:16

CSRMatrix now uses templates

63f9aba

Use std::lower_bound instead of reimplementing binary-search

752ad34

angriman force-pushed the refactor/CSRMatrix branch from ca9dd75 to 752ad34 Compare June 23, 2021 13:17

avdgrinten approved these changes Jun 30, 2021

View reviewed changes

avdgrinten merged commit 0353e6c into networkit:master Jun 30, 2021

avdgrinten deleted the refactor/CSRMatrix branch June 30, 2021 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CSRMatrix #359

Refactor CSRMatrix #359

angriman commented Jun 27, 2019 •

edited

avdgrinten left a comment

avdgrinten commented Jun 30, 2019

angriman commented Jun 30, 2019

angriman commented Jul 4, 2019

angriman commented Mar 10, 2021

fabratu left a comment

angriman commented Apr 19, 2021

avdgrinten left a comment

angriman commented Apr 20, 2021

hmeyerhenke commented Apr 21, 2021

hmeyerhenke commented Apr 21, 2021

avdgrinten commented Apr 21, 2021

hmeyerhenke commented Apr 21, 2021

angriman commented Apr 21, 2021

angriman commented Apr 22, 2021 •

edited

avdgrinten May 28, 2021

avdgrinten May 28, 2021

avdgrinten May 28, 2021

avdgrinten Jun 14, 2021

avdgrinten Jun 14, 2021

angriman Jun 16, 2021

avdgrinten Jun 16, 2021

avdgrinten Jun 14, 2021

avdgrinten Jun 14, 2021

avdgrinten Jun 14, 2021

avdgrinten Jun 14, 2021

avdgrinten Jun 14, 2021

angriman commented Jun 17, 2021

avdgrinten left a comment


		Triplet() = default;

		Triplet(index row, index column, double value) : row(row), column(column), value(value) {}

		/** Default destructor */
		virtual ~CSRGeneralMatrix() = default;

		triplets.push_back({i, j, result});
		triplets.emplace_back(i, j, result);

		triples.push_back({k, coarseIndex[j], w});
		triples.emplace_back(k, coarseIndex[j], w);

		triplets.push_back({i, j, value});
		triplets.emplace_back(i, j, value);

		triplets.push_back({i,j,val});
		triplets.emplace_back(i,j,val);

		triplets.push_back({i,j,value});
		triplets.emplace_back(i,j,value);

		triplets.push_back({v, j, dist[v]});
		triplets.emplace_back(v, j, dist[v]);

Refactor CSRMatrix #359

Refactor CSRMatrix #359

Conversation

angriman commented Jun 27, 2019 • edited

avdgrinten left a comment

Choose a reason for hiding this comment

avdgrinten commented Jun 30, 2019

angriman commented Jun 30, 2019

angriman commented Jul 4, 2019

angriman commented Mar 10, 2021

fabratu left a comment

Choose a reason for hiding this comment

angriman commented Apr 19, 2021

avdgrinten left a comment

Choose a reason for hiding this comment

angriman commented Apr 20, 2021

hmeyerhenke commented Apr 21, 2021

hmeyerhenke commented Apr 21, 2021

avdgrinten commented Apr 21, 2021

hmeyerhenke commented Apr 21, 2021

angriman commented Apr 21, 2021

angriman commented Apr 22, 2021 • edited

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

Choose a reason for hiding this comment

angriman commented Jun 17, 2021

avdgrinten left a comment

Choose a reason for hiding this comment

angriman commented Jun 27, 2019 •

edited

angriman commented Apr 22, 2021 •

edited