-
-
Notifications
You must be signed in to change notification settings - Fork 360
Implemented Thomas Algorithm in Lua #522
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
Conversation
dc256d5
to
7c86fb9
Compare
It looks good, and not mutating the input is great. We should probably ask someone (paging @Gorzoid ) who knows lua for an actual review though. |
I just based this one on the Julia implementation. |
7c86fb9
to
fce1a92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for not mutating, another +1 for avoiding extra passes for shallow copies of the parameters, looks better than the julia code imo.
I agree with jie that we should properly print the augmented matrix, otherwise its very impossible to tell what each array is just by looking at the code.
print(table.unpack(a)) | ||
print(table.unpack(b)) | ||
print(table.unpack(c)) | ||
print(table.unpack(d)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As jiegillet said, the system isn't actually this matrix. a,b and c are instead diagonals and d is the "result" column. To properly print the system we would need something like
print(b[1], c[1], "", "", "|", d[1])
print("", a[2], b[2], c[2], "|", d[2])
print("", "", a[3], b[3], "|", d[3])
I don't see much use in making code to properly any size augmented matrix so manually entering the indices should be fine.
It should be fine to use print(table.unpack(... for the solution however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I based this on the Julia implementation. I will PR a fix to that as well I guess.
2f79cdf
to
f92409d
Compare
f92409d
to
d03ccdd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got no more issues SeemsGood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems everyone is happy.
I chose not to mutate the input tables. Is that important?