Add Java implementation of Forward Euler #518
Conversation
|
I did also note that there was a PR open for this, but it's been 28 days so figured I'd do my best to help |
Gathros
added
Implementation
There was a problem hiding this comment.
Hello and thank you for the contribution!
@CDsigma is usually an active and competent member of the community, so I was willing to wait for him to come back and finish the PR with me, but my review has been pending for almost 2 weeks now and he hasn't shown up anywhere else either, so let's go with your code.
You messed up some of the indexing, probably because you worked off of the Julia code. Arrays are 1-indexed in Julia and 0-indexed in Java. The same happened to me a few weeks ago, so don't worry.
Other than that I have a few minor style complaints.
| double[] eulerResult = new double[n]; | ||
|
|
||
| //Setting the initial condition | ||
| eulerResult[1] = 1; |
There was a problem hiding this comment.
Arrays in Java are 0-indexed, so this should be eulerResult[0]. The same goes for the next line, where i should be initialized with 1.
| @@ -0,0 +1,38 @@ | |||
| public class Euler { | |||
There was a problem hiding this comment.
I'd go with ForwardEuler as the class name because there is also the Backward Euler method. Don't forget to also rename the file and adjust the import in the .md file.
| //Setting the initial condition | ||
| eulerResult[1] = 1; | ||
| for(int i = 2; i < eulerResult.length; i++) { | ||
| eulerResult[i] = eulerResult[i-1] - (3 * eulerResult[i-1] * timestep); |
There was a problem hiding this comment.
Two things in thise line:
-
The parentheses around
(3 * eulerResult[i-1] * timestep)aren't needed, although I know why you added them. They make the expression a bit more readable and I think I agree, so I guess we'll keep it. -
You didn't space out
[i-1]and again, I understand why. But in this case, I think I like[i - 1]more. What do you think?
| eulerResult[i] = eulerResult[i-1] - (3 * eulerResult[i-1] * timestep); | |
| eulerResult[i] = eulerResult[i - 1] - (3 * eulerResult[i - 1] * timestep); |
|
|
||
| for(int i = 1; i < eulerResult.length; i++) { | ||
| double time = (i - 1) * timestep; | ||
| double solution = Math.exp(-3*time); |
There was a problem hiding this comment.
Please space the contents of the parentheses of Math.exp() out.
| private static boolean checkResult(double[] eulerResult, double threshold, double timestep) { | ||
| boolean isApprox = true; | ||
|
|
||
| for(int i = 1; i < eulerResult.length; i++) { |
There was a problem hiding this comment.
Same as above. Arrays start at index 0.
|
You're spot on with why my indexing mistakes were there! I also incorporated all of the style changes, let me know if I missed anything or if you notice something else! |
| boolean isApprox = true; | ||
|
|
||
| for(int i = 0; i < eulerResult.length; i++) { | ||
| double time = (i - 1) * timestep; |
There was a problem hiding this comment.
I found another error that leads back to the indexing difference between Julia and Java. time is just i * timestep.
| @@ -14,7 +14,7 @@ private static boolean checkResult(double[] eulerResult, double threshold, doubl | |||
| boolean isApprox = true; | |||
|
|
|||
| for(int i = 0; i < eulerResult.length; i++) { | |||
| double time = (i - 1) * timestep; | |||
| double time = (i) * timestep; | |||
There was a problem hiding this comment.
Uhm... So, why exactly is i still in parentheses? =P
There was a problem hiding this comment.
Ha! Because I'm lazy and didn't even look that closely
Edits are fine, but please provide any feedback you can so I can make my next PRs better