Implement Gaussian Elimination in C++ #258

mika314 · 2018-07-16T07:29:41Z

No description provided.

mika314 · 2018-07-16T16:42:56Z

chapters/algorithms/gaussian_elimination/gaussian_elimination.md

@@ -259,6 +259,8 @@ In code, this looks like:
 [import:1-42, lang:"julia"](code/julia/gaussian_elimination.jl)
 {% sample lang="c" %}
 [import:13-44, lang:"c_cpp"](code/c/gaussian_elimination.c)
+{% sample lang="cpp" %}
+[import:6-51, lang:"c_cpp"](code/c++/gaussian_elimination.cpp)


Link to back_substitution function is missing

leios · 2018-07-17T08:08:45Z

Hey, sorry for this @AntonTe (I know you have been doing a lot of updates and appreciate it), but we just changed the gaussian elimination code to include a gauss-jordan function to be a bit more general, see #219

We're getting to all the PR's sorry it takes so long!

mika314 · 2018-07-17T14:54:22Z

@leios no problems, I'll redo implementation this night. Closing the PR. I'll reopen it after redoing the implementation.

mika314 · 2018-07-18T04:09:00Z

Pushed version with gaussian_jordan

Gathros · 2018-07-21T14:26:35Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+  // Main loop going through all columns
+  for (int col = 0; col < cols - 1; ++col) {
+    // Step 1: finding the maximum element for each column
+    int max_index = [&]() {


The lambda function doesn't help with readability and there is no need too.

This technique called IIFE stands for an immediately-invoked function expression, it was popularized in JS and adopted in C++. It has some benefits comparing with old style:

initialize the variable at declaration

keep local var in the lambda in the scope

Fair enough, but it's still hard to read. I've asked around on discord and people agree.

Sorry, my English is my second language. "People agree" with "fair enough" or "it is hard to read"?

And what is the final decision?

Sorry that was a bad statement I wrote. You have a fair point but I have asked around and people think it's hard to read.

Your suggestion is conflicting with C++ Core Guidelines: ES.5: Keep scopes small, ES.20: Always initialize an object. To avoid this without using lambda I would need to create named function.

The lambda function makes it hard for people to understand your c++ code if they are new to the language. Also my C code wasn't long:

for (size_t i = k + 1; i < rows; ++i) { if (fabs(a[i * cols + k]) > fabs(a[pivot * cols + k])) { pivot = i; } }

Let's wait what @leios would say.

Modern C++ uses lambda quite aggressively it was introduces in C++ 11 more than 7 years ago

Without IIFE it would be hard to follow C++ Core Guidelines

To be clear, we are comparing the following code samples:

for (int col = 0; col < cols - 1; ++col) { // Step 1: finding the maximum element for each column int max_index = [&]() { int res = row; int max_element = a[col + row * cols]; for (int r = row + 1; r < rows; ++r) { if (max_element < std::abs(a[col + r * cols])) { max_element = std::abs(a[col + r * cols]); res = r; } } return res; }(); ... }

and

for (int col = 0; col < cols - 1; ++col) { // Step 1: finding the maximum element for each column int max_index = col + row * cols; for (size_t i = col + 1; i < rows; ++i) { if (fabs(a[i * cols + col]) > fabs(a[max_index * cols + col])) { max_index = i; } } ... }

In my opinion, the latter one is easier to read (unless I screwed something up and this doesn't actually do the right action). I feel I am not fully understanding the conversation. @AntonTe How does the second code snippet violate the core guidelines? I don't think it adds another variable to the scope and doesn't violate ES.20.

@leios you right. I'll make changes. The problem in my code is not about using IIFE, but about caching max_element.

Gathros · 2018-07-21T14:27:24Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+    int max_index = [&]() {
+      int res = row;
+      int max_element = a[col + row * cols];
+      for (int r = row + 1; r < rows; ++r)


The for loop has no curly bracket.

Will fix it, I did not know about it. Do we have any style guide document, so in the future I would not make this sort of mistakes? Like snake_case and enforced curly brackets?

Not really but I do like snake case and I think we should enforce curly brackets since it's are clearer.

Gathros · 2018-07-21T14:28:09Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+#include <vector>
+
+void gaussian_elimination(std::vector<double>& a, int cols) {
+  assert(a.size() % cols == 0);


no need for assert.

I agree we should not assert on user input, but what you can offer instead?

I can see multiple options:

remove assert: the code will crash later of even worse return incorrect result

log the error and crash: same as assert but more lines of code

return the error code: for production solution it might be OK, but it requires a lot of boiler plate code, and hides the important parts of the code

throw an exception: for production solution it might be OK, but it requires some of boiler plate code, and hides the important parts of the code

Conclusion: for this particular case, sample code for the book about algorithms I prefer to use assert.

Have an if statement there like if (a.size() % cols != 0) {.

Gathros · 2018-07-21T14:28:32Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+}
+
+int main() {
+  std::vector<double> a = {2, 3, 4, 6, 1, 2, 3, 4, 3, -4, 0, 10};


Make this the same as in the julia code.

Gathros · 2018-07-21T14:29:10Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+  for (int i = 0; i < rows; ++i) {
+    std::cout << "[";
+    for (int j = 0; j < cols; ++j) {
+      std::cout << a[j + i * cols] << " ";


use "\t" and not a space for better formatting.

Gathros · 2018-07-21T14:29:54Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+    if (a[col + row * cols] != 0) {
+
+      // divide row by pivot and leaving pivot as 1
+      for (int i = cols - 1; i >= static_cast<int>(col); --i)


why cast col from int to int.

Gathros · 2018-07-21T14:31:51Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+  std::vector<double> soln(rows);
+
+  // initialize the final element
+  soln[rows - 1] =


you don't need this.

I copied from Jula code:

# initialize the final element soln[rows] = A[rows, cols] / A[rows, cols-1]

Is Jula code has a bug?

I'll tell @leios. But looking at the for loop, when i is rows - 1 sum will be 0. This is because the first for loop will just be jumped making that line useless since it will happen later.

I should do better job or reading and analyzing the code. Need to run for grocery... will back soon.

I found the bug, it is next line; should be -2 instead of -1.

It looks better if you remove that line.

You right. I'll make a pull request for Julia code as well.

leios · 2018-07-21T23:26:38Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+        a[i + row * cols] /= a[col + row * cols];
+
+      // subtract value from above row and set values above pivot to 0
+      for (int i = 0; i < static_cast<int>(row - 1); ++i)


I think this should just be row instead of row - 1, otherwise gauss-jordan does not put the matrix into rref.

Also: are the static_cast<int>s necessary here? It's no big deal if they are, I'm just trying to figure out why they are here.

man... so many static_casts... :(

row - 1 looks right, we are not touching current row we only updating all rows above.

So, when running this with row, we get the right answer:

[1.000000 0.000000 0.000000 1.636364 ] [0.000000 1.000000 0.000000 -1.272727 ] [0.000000 0.000000 1.000000 1.636364 ]

but with row - 1, we get:

[1.000000 -1.333333 0.000000 3.333333 ] [0.000000 1.000000 0.705882 -0.117647 ] [0.000000 0.000000 1.000000 1.636364 ]

which means we are ignoring the row just above the row we want too.

as far as the static_casts are concerned, if there's a reason to leave them in, it's no problem. I was more curious about it than anything.

You right, classic off-by-one error.

There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors.

You can see static_cast in the code, because originally it was explosive mix of unsigned and signed integer, I refactored code to use only signed integers but forgot to remove static_cast.

Gathros · 2018-08-03T14:56:31Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+                            1, 2, 3, 4,
+                            3, -4, 0, 10 };
+  const int cols = 4;
+  if (a.size() % cols != 0)


Why not have the if statement in the other functions replacing assert. People are going to take them and not the main function when they need a Gaussian Elimination.

Above I explained different solutions instead of assert, majority of solutions implied using if. So it is not clear for me what you want to put inside if-statement? As I mentioned above:

log the error and crash

return the error code

throw an exception
?

I think you should get rid of all asserts and not even replace them with ifs. We can just always assume that the input is correct for the sake of simplicity.
If you still want to have some checks, then inside the if you can have return 0; or a zero matrix or something.

jiegillet · 2018-08-04T13:10:22Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+    }
+
+    // Step 2: swap row with highest value for that column to the top
+    for (int c = 0; c < cols; ++c)


You should check if row== max_index , if it's the case you don't need to swap.

- add curly bracket - remove static_cast - remove IIFE - fix format for the matrix - format output with tabs - properly sanitize user input - remove extraneous assignment - fix off-by-1

- remove asserts - don't swap if it is a same row

Gathros · 2018-08-26T10:09:44Z

contents/gaussian_elimination/code/c++/gaussian_elimination.cpp

+        a[i + row * cols] /= a[col + row * cols];
+
+      // subtract value from above row and set values above pivot to 0
+      for (int i = 0; i < row; ++i)


Add brackets to this for loop.

leios · 2018-10-03T21:02:31Z

Hey guys, what is the status on this PR?

leios · 2018-12-23T23:42:11Z

I think there were only a few smallscale changes requested on this PR, but we have another Gaussian Elimination PR now. If this one is stale, we'll close it and work on the next one.

Gathros · 2019-01-04T11:43:55Z

This pull request is stale, so I will close it. You had months (since Oct 3) to reply and in that time you've been online on github.

Gathros added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Jul 16, 2018

mika314 commented Jul 16, 2018

View reviewed changes

mika314 force-pushed the gaussian_elimination_cpp branch from d532840 to 2a1d6e4 Compare July 17, 2018 03:40

mika314 closed this Jul 17, 2018

mika314 reopened this Jul 18, 2018

mika314 force-pushed the gaussian_elimination_cpp branch from 2a1d6e4 to 7d906a5 Compare July 18, 2018 04:08

mika314 force-pushed the gaussian_elimination_cpp branch 4 times, most recently from 6dbad83 to 5cf1fc3 Compare July 20, 2018 05:32

Gathros requested changes Jul 21, 2018

View reviewed changes

mika314 force-pushed the gaussian_elimination_cpp branch 4 times, most recently from 2125f5c to 55f0195 Compare July 21, 2018 21:45

leios reviewed Jul 21, 2018

View reviewed changes

mika314 force-pushed the gaussian_elimination_cpp branch 3 times, most recently from 8a63475 to 60b5d71 Compare July 22, 2018 00:29

Gathros requested changes Aug 3, 2018

View reviewed changes

jiegillet reviewed Aug 4, 2018

View reviewed changes

mika314 added 3 commits August 4, 2018 21:57

Implement Gaussian Elimination in C++

2f52647

Address code review comments:

e843418

- add curly bracket - remove static_cast - remove IIFE - fix format for the matrix - format output with tabs - properly sanitize user input - remove extraneous assignment - fix off-by-1

Address code review comments

dc87fe0

- remove asserts - don't swap if it is a same row

mika314 force-pushed the gaussian_elimination_cpp branch from 60b5d71 to dc87fe0 Compare August 5, 2018 05:03

Gathros requested changes Aug 26, 2018

View reviewed changes

leios mentioned this pull request Dec 23, 2018

"Gaussian Elimination" Implementation in C++ #555

Merged

Gathros closed this Jan 4, 2019

Implement Gaussian Elimination in C++ #258

Implement Gaussian Elimination in C++ #258

Conversation

mika314 commented Jul 16, 2018

Choose a reason for hiding this comment

leios commented Jul 17, 2018

mika314 commented Jul 17, 2018

mika314 commented Jul 18, 2018

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

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

mika314 Jul 21, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leios commented Oct 3, 2018

leios commented Dec 23, 2018

Gathros commented Jan 4, 2019

mika314 Jul 21, 2018 •

edited

Loading