Skip to content

Clean up Java implementation of Thomas Algorithm #496

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 7 commits into from
Oct 13, 2018

Conversation

Butt4cak3
Copy link
Contributor

@Butt4cak3 Butt4cak3 commented Oct 11, 2018

When merged, this PR fixes a few issues with the Java implementation of the Thomas algorithm and also changes it so that it works similarly to the new Julia implementation (#495).

  • The class and file name were thomas, but classes in Java should be PascalCase, so I renamed them to Thomas.

  • The thomasAlgorithm method had a int size parameter, which is not needed in Java. I assume that the code was directly ported from the C implementation and that was not taken out.

  • I had a short talk with @leios on stream today. I mentioned how almost none of the implementations return the result. Instead, they all modify the input vector, which they get passed by reference. The new version of this does not modify any of the input vectors and returns the output vector.

  • The output was quite messy, also as a result of the fact that the code was directly ported over from C.

  • All the array declarations were formatted as double x[] instead of double[] x. The latter is, as far as I know, the preferred way in Java. This is probably also an artifact of the port from C.

  • Lastly, I made the output dynamic. The output is hard-coded in almost all implementations and I think we should change that. It's not that difficult.

Note: I don't know why GitHub reports thomas.py as deleted and Thomas.py as new. I used git mv to rename the file.

Classes in Java are usually PascalCase and the file names have to match
- There were lots of unnecessary newlines
- Some of the output was hardcoded
@Butt4cak3 Butt4cak3 added the Implementation Edit This provides an edit to an algorithm implementation. (Code and maybe md files are edited.) label Oct 11, 2018
@jiegillet
Copy link
Member

I support returning a result over modifying the input. I've been requesting that feature on the Thomas algorithm PRs that I reviewed.

@PudottaPommin PudottaPommin self-assigned this Oct 12, 2018
@berquist
Copy link
Member

Note: I don't know why GitHub reports thomas.py as deleted and Thomas.py as new. I used git mv to rename the file.

If the move also contains enough of a percentage changed over some threshold (can't remember the number), Git views it as a delete and new, rather than a rename.

Copy link
Member

@PudottaPommin PudottaPommin left a comment

Choose a reason for hiding this comment

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

Hey, looks good and works like charm.

I have only one question (in code).
I mark this for now as Request changes.

@PudottaPommin PudottaPommin merged commit 54b833b into master Oct 13, 2018
@Butt4cak3 Butt4cak3 deleted the thomas-java-cleanup branch October 14, 2018 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implementation Edit This provides an edit to an algorithm implementation. (Code and maybe md files are edited.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants