1D wave equation solver.[WIP] #16

Balavarun5 · 2017-09-08T08:24:48Z

Code to simulate the time evolution of the 1D wave equation using discontinuous Galerkin and Euler forward methods. This code is not completely functional (Ref- Issue #6 for further details), as the amplitude increases with time and eventually blows up.

…form a movie.

…ime to N_LGL x N_Elements x time

…speed. -testwaveEqn has issues accessing modules.

…ecreases with larger array size).

amanabt

This review is mainly concerned with the code guidelines issues. I have not reviewed few changes related to writing mathematical equations in doc-strings because I had some confusion about it, which, I have discussed in issue #19. I will review them once this issue is resolved.

I did not review any of the test functions because on running pytest, the tests are failing.

amanabt · 2017-09-09T04:48:38Z

README.md

@@ -27,4 +27,4 @@ python3 main.py
 * **Mani Chandra** - [GitHub Profile](https://github.com/mchandra)



We updated the README a few days ago. You should have the updated README here. I am attaching the
link where you can find it.
hdf5_ImplementationForStoring_u

amanabt · 2017-09-09T04:52:36Z

code/app/global_variables.py

@@ -1,201 +1,72 @@
 import numpy as np


According to the Quazartech style guidelines, the imports should be grouped in the following order
Imports should be grouped in the following order:

standard library imports

related third party imports

local application/library specific imports

I would prefer if you give spacing between different groups.
I am adding this review only here but you should check for this issue in every python file.

amanabt · 2017-09-09T05:06:58Z

code/app/global_variables.py

-    dLp_xi = af.blas.matmul(differentiation_coeffs, nodes_power_tile)
-
-    return dLp_xi
+


why are you still using global variables?
You mentioned in Issue #17 that this code is not using global variables. What's going on?

amanabt · 2017-09-09T05:28:29Z

code/app/global_variables.py

-                      stored in stored in variable `time`.
-
-
+    Calculates and returns the LGL points for a given N, These are roots of


You can write,

Calculates : math: `N` Legendre-Gauss-Lobatto (LGL) points. LGL points are the roots of the polynomial :math: `(1 - \\xi ** 2) P_{n - 1}'(\\xi) = 0` Where :math: `P_{n}(\\xi)` are the Legendre polynomials. This function finds the roots of the above polynomial

amanabt · 2017-09-09T05:29:01Z

code/app/global_variables.py

+    Calculates and returns the LGL points for a given N, These are roots of
+    the formula.
+    :math: `(1 - xi ** 2) P_{n - 1}'(xi) = 0`
+    Legendre polynomials satisfy the recurrence relation


What is the point adding this recurrence relation?

The variable legendre_N_minus_1 uses this recurrence relation.

It would have made sense to give the recurrence relation if you had explained the algorithm. But since you have not explained the algorithm, the reader won't know why you have written the recurrence relation.

amanabt · 2017-09-09T10:55:55Z

code/app/wave_equation.py

+
+    Parameters
+    ----------
+    u_t_n : arrayfire.Array [N_LGL N_Elements 1 1]


Why is the variable named u_t_n?
If I understand correctly, u_t_n is basically the wave amplitude at n^{th} time step.
Then why don't we name it just u_n.

amanabt · 2017-09-09T10:57:08Z

code/app/wave_equation.py

+
+def surface_term(u_t_n):
+    '''
+    A function which is used to calculate the surface term,


Don't start with A function ..., we know that it is a function. Remove unneeded words.

amanabt · 2017-09-09T11:56:42Z

code/unit_test/test_waveEqn.py

@@ -1,72 +1,41 @@
 import numpy as np


Code guidelines violation for imports.

amanabt · 2017-09-09T11:57:15Z

code/unit_test/test_waveEqn.py

 from utils import utils
-import math
+af.set_backend('opencl')


I think that this function should be called right after importing arrayfire.

amanabt · 2017-09-09T12:02:03Z

code/app/wave_equation.py

+    ---------
+    Link to PDF describing the algorithm to obtain the surface term.
+
+    https://cocalc.com/projects/1b7f404c-87ba-40d0-816c-2eba17466aa8/files


See issue #18 and shorten the URL. Do this at all the places where such long URLs are written.

amanabt · 2017-09-11T09:01:17Z

@Balavarun5 @mchandra
Changing the global variables will make the code very different and reviewing the PR might become more complicated. Should we remove the global variable in a PR following this one?

@Balavarun5 have you already removed the usage of global variables from the code?

…e refactored to include `Integrate()` function.

…umentation and comments need to be added.

Balavarun5 added 17 commits August 1, 2017 11:05

Push changes into lax_friedrichs_flux

ea3a7b5

Added functions to obtain the lax-friedrichs-flux and surface term.

b8526a2

Time evolution of wave equation obtained.

3f92f4a

Simulating for higher wave speeds.

1014ef4

The images would be created in 1D_Wave_images folder, stitch them to …

c5d0a2f

…form a movie.

Changed amplitude from being represented as an N_Elements x N_LGL x t…

bc14e39

…ime to N_LGL x N_Elements x time

Resolving wave speed abnormality.

0011aaf

-Wave is moving at the expected speed by taking c_lax to be the wave …

64f414c

…speed. -testwaveEqn has issues accessing modules.

Unit tests working.

7507b3d

Unit tests working.

756c8f6

Edits in the documentation.

bdffc6c

Minor edits

88b04d0

Documentations edited. Tabs converted to spaces.

6c5c3d0

Cleanup of the code, Unit tests working. Iteration speed unchanged (d…

4b5b4a7

…ecreases with larger array size).

Minor edits in documentation of functions.

8d38457

Change in note for developers in readme.md

457f42d

Merge branch 'master' into lax_friedrichs_flux

28729c3

Balavarun5 changed the title ~~Lax friedrichs flux~~ 1D wave equation solver. Sep 8, 2017

Balavarun5 requested review from mchandra and amanabt September 8, 2017 09:53

Balavarun5 self-assigned this Sep 8, 2017

Balavarun5 mentioned this pull request Sep 8, 2017

Usage of global variables in the code. #17

Closed

amanabt added this to the 1D Wave equation solver. milestone Sep 9, 2017

amanabt requested changes Sep 9, 2017

View reviewed changes

amanabt changed the title ~~1D wave equation solver.~~ 1D wave equation solver.[WIP] Sep 9, 2017

Balavarun5 mentioned this pull request Sep 15, 2017

Inconsistencies in calculation of A-matrix using Gauss-Lobatto quadrature #13

Closed

Balavarun5 added 3 commits September 15, 2017 17:50

Temporary commit. Not all changes have been implemented. Code is to b…

f82df88

…e refactored to include `Integrate()` function.

Code reffactored to use Integrate() function. Unit tests passing. Doc…

e6b5c71

…umentation and comments need to be added.

Edited the documentation. Examples need to be added.

f79ceb9

mchandra merged commit ba250f5 into QuazarTech:master Sep 18, 2017

Balavarun5 mentioned this pull request Sep 19, 2017

Enhancement for the Integrate() function #25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1D wave equation solver.[WIP] #16

1D wave equation solver.[WIP] #16

Balavarun5 commented Sep 8, 2017 •

edited

Loading

amanabt left a comment •

edited

Loading

amanabt Sep 9, 2017

amanabt Sep 9, 2017

amanabt Sep 9, 2017

amanabt Sep 9, 2017

amanabt Sep 9, 2017

Balavarun5 Sep 9, 2017

amanabt Sep 9, 2017

amanabt Sep 9, 2017

amanabt Sep 9, 2017

amanabt Sep 9, 2017

amanabt Sep 9, 2017

amanabt Sep 9, 2017

amanabt commented Sep 11, 2017

		@@ -27,4 +27,4 @@ python3 main.py
		* Mani Chandra - [GitHub Profile](https://github.com/mchandra)

		dLp_xi = af.blas.matmul(differentiation_coeffs, nodes_power_tile)

		return dLp_xi

		stored in stored in variable `time`.


		Calculates and returns the LGL points for a given N, These are roots of

1D wave equation solver.[WIP] #16

1D wave equation solver.[WIP] #16

Conversation

Balavarun5 commented Sep 8, 2017 • edited Loading

amanabt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanabt commented Sep 11, 2017

Balavarun5 commented Sep 8, 2017 •

edited

Loading

amanabt left a comment •

edited

Loading