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

alternate coo2csr implementation #175

Closed
wants to merge 13 commits into from
Closed

Conversation

superwhiskers
Copy link
Collaborator

@superwhiskers superwhiskers commented Jul 5, 2024

this is a new coo2csr implementation, similar to the work done in #174, but written entirely from scratch, presumably with better performance (space/runtime---i've yet to benchmark it).

@superwhiskers superwhiskers requested a review from pelesh July 5, 2024 18:15
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

This is an incomplete review catching a few places that may be bugs and suggesting how to make the code more readable.

resolve/matrix/Utilities.cpp Outdated Show resolved Hide resolved
resolve/matrix/Utilities.cpp Outdated Show resolved Hide resolved
resolve/matrix/Utilities.cpp Outdated Show resolved Hide resolved
Comment on lines 113 to 115
if (new_columns[insertion_pos] == columns[i]) {
new_values[insertion_pos] = values[i];
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it assignment here? Shouldn't it be adding in place?

Suggested change
if (new_columns[insertion_pos] == columns[i]) {
new_values[insertion_pos] = values[i];
} else {
if (new_columns[insertion_pos] == columns[i]) {
new_values[insertion_pos] += values[i];
} else {

Comment on lines 106 to 111
index_type insertion_pos =
static_cast<index_type>(
std::lower_bound(&new_columns[new_rows[rows[i]]],
&new_columns[new_rows[rows[i]] + used[rows[i]]],
columns[i])
- new_columns);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider rewriting this as:

  // Find first instance of columns[i] in the range from ...
  auto lb = std::lower_bound(&new_columns[new_rows[rows[i]]],
                             &new_columns[new_rows[rows[i]] + used[rows[i]]],
                             columns[i]);
  // Substract number of ... from the lower bound lb
  lb -= new_columns;
  index_type insertion_pos = static_cast<index_type>(lb);

Also, it would be helpful to add comments before lines that are not quite obvious to explain what is done there. Perhaps using more intuitive variable names and adding comments before variable definition could help make code easier to read.

resolve/matrix/Utilities.cpp Outdated Show resolved Hide resolved
resolve/matrix/Utilities.cpp Outdated Show resolved Hide resolved
@superwhiskers
Copy link
Collaborator Author

all of the comments above should be addressed now

This was referenced Jul 24, 2024
@pelesh pelesh marked this pull request as draft August 17, 2024 11:22
@pelesh
Copy link
Collaborator

pelesh commented Sep 17, 2024

Closing in favor of #184.

@pelesh pelesh closed this Sep 17, 2024
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.

2 participants