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

fixed both ryser algs, added tests, updated tuning #24

Closed
wants to merge 1 commit into from

Conversation

cassmasschelein
Copy link
Collaborator

Fixed the square Ryser algorithm, updated the rectangle Ryser algorithm, added code for second svm to update the tuning function to get out 8 parameters instead of the original 4, added cmake fix to pyproject.toml, added more pytests

Copy link
Collaborator

@msricher msricher left a comment

Choose a reason for hiding this comment

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

Thanks!!! Just a few things. Use std::popcount from <bits> for counting bits everywhere, and initialize variables as close to using them as possible (limit the scope of a bunch of i, j, ks, etc.)

return n ^ (n >> 1);
}

inline int count_bits (int n) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use std::popcount from <bits>.

rowsum += ptr[m * i + j];
(void)n;

size_t i, j, k;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Initialize variables at the deepest scope possible. Like in the for loops, for (size_t i = 0; .....). Check this generally. I cleaned it up most other places.

/* Update product of row sums. */

rowsumprod *= rowsum;
out += rowsumprod * (1 - ((__builtin_popcountll(k) & 1) << 1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use std::popcount here too. It's new in C++20, which we're using... it's best to avoid these compiler extensions if possible.

@@ -38,6 +38,9 @@ test = ["pytest"]
[tool.scikit-build]
wheel.expand-macos-universal-tags = true

[tool.scikit-build]
cmake.args = ["-DPERMANENT_TUNE=ON"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Set this to OFF by default.

@cassmasschelein
Copy link
Collaborator Author

I have made theses changes, will make another pull request

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