Skip to content

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

Merged
merged 1 commit into from
Oct 20, 2018

Conversation

Vexatos
Copy link
Contributor

@Vexatos Vexatos commented Oct 20, 2018

I chose not to mutate the input tables. Is that important?

@Vexatos Vexatos force-pushed the thomas-algo-lua branch 2 times, most recently from dc256d5 to 7c86fb9 Compare October 20, 2018 09:53
@june128 june128 added Hacktoberfest The label for all Hacktoberfest related things! Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) labels Oct 20, 2018
@jiegillet
Copy link
Member

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.
One problem is how you print the system, it doesn't really work since a b and c are diagonal values of the matrix, not rows (and I realized you copied the Julia code, paging @leios ). If you want to prettify it, you can do something like the PHP implementation.

@Vexatos
Copy link
Contributor Author

Vexatos commented Oct 20, 2018

I just based this one on the Julia implementation.

Copy link
Contributor

@Gorzoid Gorzoid left a 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))
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@Vexatos Vexatos force-pushed the thomas-algo-lua branch 2 times, most recently from 2f79cdf to f92409d Compare October 20, 2018 21:52
Copy link
Contributor

@Gorzoid Gorzoid left a 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

Copy link
Member

@leios leios left a 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.

@leios leios merged commit f3f28b9 into algorithm-archivists:master Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest The label for all Hacktoberfest related things! Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants