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

Compile numerics #516

Merged
merged 3 commits into from
Feb 23, 2021
Merged

Compile numerics #516

merged 3 commits into from
Feb 23, 2021

Conversation

angriman
Copy link
Member

@angriman angriman commented Mar 4, 2020

The numerics module and its tests were never compiled because numerics was not included in networkit/CMakeLists. This PR fixes some compilation errors in the numerics module and includes it in the build and updates googletest.

Unfortunately, LAMGGTest segfaults when compiled with clang-3.8/3.8.1. As far as I see, this is due to a compiler bug. Since clang-3.8 has another bug that prevents us to generalize CSRMatrix to store generic types (see #359), I'd suggest to move to clang-3.9.

@angriman angriman changed the base branch from Dev to master May 29, 2020 16:53
@angriman angriman force-pushed the compile-numerics branch 4 times, most recently from b917e6a to b5c6348 Compare February 18, 2021 15:13
@avdgrinten
Copy link
Contributor

Updates of Clang versions should go into a separate PR (which we would merge first).

include/networkit/io/LineFileReader.hpp Show resolved Hide resolved
networkit/cpp/centrality/test/CentralityGTest.cpp Outdated Show resolved Hide resolved
networkit/cpp/graph/test/GraphBuilderAutoCompleteGTest.cpp Outdated Show resolved Hide resolved
networkit/cpp/graph/test/GraphBuilderDirectSwapGTest.cpp Outdated Show resolved Hide resolved
networkit/cpp/numerics/test/LAMGGTest.cpp Outdated Show resolved Hide resolved
networkit/cpp/numerics/LAMG/MultiLevelSetup.cpp Outdated Show resolved Hide resolved
@angriman angriman force-pushed the compile-numerics branch 5 times, most recently from 602367e to 8f64a7f Compare February 22, 2021 12:40
Add numerics module to CMakeLists, fix compilation errors in the
numerics module.
Fix unused parameter warning, and minor changes.
fabratu
fabratu previously approved these changes Feb 23, 2021
Copy link
Member

@fabratu fabratu left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@avdgrinten avdgrinten left a comment

Choose a reason for hiding this comment

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

I am unhappy with the structure of this PR. Why does this change stuff in io, graph, algebraic, etc. if the goal is to compile the numerics code?

@angriman
Copy link
Member Author

Sorry those have been introduced by mistake, the changes in algebraic are needed because that module is used by numerics.



Vector LAMGGTest::randVector(count dimension, double lower, double upper) const {
Vector LAMGGTest::randVector(count dimension, double, double) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks suspicious. Why doesn't the function use it's arguments?

Copy link
Member Author

@angriman angriman Feb 23, 2021

Choose a reason for hiding this comment

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

I don't know the details about that, I just removed unused variables.

Choose a reason for hiding this comment

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

Looks like the originally wanted functionality was removed without removing the parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly. In that case, remove the parameters as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed them.

Copy link
Contributor

@avdgrinten avdgrinten left a comment

Choose a reason for hiding this comment

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

LGTM now.

@fabratu
Copy link
Member

fabratu commented Feb 23, 2021

Merging this early since this (mostly) only adds a new module and is needed for the new release.

@fabratu fabratu merged commit 48162bf into networkit:master Feb 23, 2021
@angriman angriman deleted the compile-numerics branch March 23, 2022 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants