Skip to content

add thomas algorithm in javascript #491

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

Closed
wants to merge 5 commits into from
Closed

add thomas algorithm in javascript #491

wants to merge 5 commits into from

Conversation

Yordrar
Copy link
Contributor

@Yordrar Yordrar commented Oct 9, 2018

No description provided.

@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 9, 2018

thomas(a, b, c, x);

for (let i = 0; i < 3; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use for (const value of x) { here. Also, you could just print the array as a whole: console.log("has the solution: ", x)

Copy link
Contributor Author

@Yordrar Yordrar Oct 9, 2018

Choose a reason for hiding this comment

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

done, went for the console.log("solution: ", x) as it is simpler

@Butt4cak3 Butt4cak3 self-assigned this Oct 11, 2018
Copy link
Contributor

@Butt4cak3 Butt4cak3 left a comment

Choose a reason for hiding this comment

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

As always, thanks for the contribution!

We now have a code style guide for JavaScript. You should check that out because there are a few ways your code deviates from it. The most obvious thing is that your code is indented with tabs instead of 2 spaces. The rest is explained in the comments.

Other than that, I just suggested to @leios on stream that we should probably make the implementations return the result instead of modifying the input. He already changed the Julia implementation on stream (#495).

@@ -0,0 +1,28 @@
function thomas(a, b, c, x) {
let y = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be const.

x[0] = x[0] / b[0];

for (let i = 1; i < a.length; i++) {
let scale = 1.0 / (b[i] - a[i] * y[i - 1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be const.

You also don't have to use 1.0. JavaScript doesn't differentiate between 1 and 1.0.

@Yordrar
Copy link
Contributor Author

Yordrar commented Oct 13, 2018

everything corrected, @Butt4cak3

@Butt4cak3
Copy link
Contributor

I'll take a look at it tomorrow. I didn't have access to my PC for most of today.

@Butt4cak3
Copy link
Contributor

You're still modifying the input (x). You're also returning it at the end, but that's not what I meant. If you looks at the Julia implementation I mentioned in my first review or the current Java implementation you will see that they create a new array for the solution and modify that one instead.

We're still discussing whether we should actually make this a requirement (#510), but by the looks of it, we will. If you want, you can hold off with this PR until the issue is resolved or you can just go ahead and do it now. Your call.

@Yordrar
Copy link
Contributor Author

Yordrar commented Oct 14, 2018

@Butt4cak3 , I corrected it just in case, if we then decide that pure functions shouldn't be compulsory and the code is more readable without a pure function I'll change it again, no prob. (I think it is more readable without it being pure but that's discussion for #510 :P)

@Butt4cak3
Copy link
Contributor

Just to clarify: I'm still waiting for that discussion to resolve. I didn't forget your PR. I just don't want us to have to do the work twice.

@berquist
Copy link
Member

We are coming up on one calendar year since the last comment. Is there anything left to be done?

@Butt4cak3
Copy link
Contributor

I'll stop trying to fool myself and others. I'm just not active enough in this community anymore. Any PRs that are currently assigned to me should be handled by someone else. I don't even know what these things are about anymore.

@Butt4cak3 Butt4cak3 removed their assignment Nov 27, 2019
@leios
Copy link
Member

leios commented May 5, 2020

This has been repurposed into #683

@leios leios closed this May 5, 2020
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.

6 participants