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

Fix loads of things #185

Merged
merged 52 commits into from
Sep 23, 2024
Merged

Fix loads of things #185

merged 52 commits into from
Sep 23, 2024

Conversation

larsgottesbueren
Copy link
Member

Make includes compatible with some non-standard build systems 😔
Some UB and ASAN fixes in uncoarsening and priority queue
Some WHFC assertion fixes + underflow

@SebastianSchlag
Copy link
Member

Some WHFC assertion fixes + underflow

Should we pull those into KaHyPar as well?

Copy link
Member

@SebastianSchlag SebastianSchlag left a comment

Choose a reason for hiding this comment

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

LGTM

@larsgottesbueren
Copy link
Member Author

Some WHFC assertion fixes + underflow

Should we pull those into KaHyPar as well?

There is no immediate need. The changes affect bulk piercing and push-relabel, both of which are not used in KaHyPar at the moment.

Copy link
Collaborator

@N-Maas N-Maas left a comment

Choose a reason for hiding this comment

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

Looks good, except for cleaning up external_tools (see comment)

@@ -763,7 +763,7 @@ namespace mt_kahypar::io {
std::sort(internal_pins.begin(), internal_pins.end());
std::sort(internal_degree.begin(), internal_degree.end());

auto square = [&](size_t x) { return x * x; };
auto square = [&](double x) { return x * x; };
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually no UB since unsigned overflow is well defined, but of course produces wrong results^^


#include "mt-kahypar/partition/refinement/gains/gain_definitions.h"
#include "mt-kahypar/utils/cast.h"
#include "mt-kahypar/partition/context.h"

#include "pcg_random.hpp"

Copy link
Collaborator

Choose a reason for hiding this comment

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

This means we can also remove pcg from external_tools, right? It would be good to also include this in the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Member

@kittobi1992 kittobi1992 left a comment

Choose a reason for hiding this comment

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

only some docu and style nitpicks

@@ -87,7 +87,9 @@ class ConnectivitySets {
_shallow_copy_bitset() {
if ( num_hyperedges > 0 ) {
_bits.resize("Refinement", "connectivity_set",
static_cast<size_t>(num_hyperedges) * _num_blocks_per_hyperedge, true, assign_parallel);
static_cast<size_t>(num_hyperedges) * _num_blocks_per_hyperedge
+ 1 // The nextBlockID() implementation performs a (masked out) load past the end
Copy link
Member

Choose a reason for hiding this comment

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

Or this way or remove the comment:

Suggested change
+ 1 // The nextBlockID() implementation performs a (masked out) load past the end
+ 1 /* The nextBlockID() implementation performs a (masked out) load past the end */

@@ -195,7 +194,10 @@ namespace mt_kahypar {
template<typename TypeTraits>
void MultilevelUncoarsener<TypeTraits>::refineImpl() {
PartitionedHypergraph& partitioned_hypergraph = *_uncoarseningData.partitioned_hg;
const double time_limit = Base::refinementTimeLimit(_context, (_uncoarseningData.hierarchy)[_current_level].coarseningTime());
double time_limit = std::numeric_limits<double>::max();
if (_current_level >= 0 && _current_level != _num_levels) { // there is a refinement run on the coarsest graph before projection, there is no value stored for this run.
Copy link
Member

Choose a reason for hiding this comment

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

put comment into next line

return 0;
}

HyperedgeWeight distance(const PartitionID , const PartitionID ) const {
Copy link
Member

Choose a reason for hiding this comment

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

Formatting (also apply to other functions)

Suggested change
HyperedgeWeight distance(const PartitionID , const PartitionID ) const {
HyperedgeWeight distance(const PartitionID, const PartitionID) const {

@larsgottesbueren larsgottesbueren merged commit 39e06c7 into master Sep 23, 2024
16 checks passed
@larsgottesbueren larsgottesbueren deleted the tbb_includes branch September 23, 2024 11:47
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.

4 participants