examples: Add a notebook with a toy variable-z topography implementation #1900
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov Report
@@ Coverage Diff @@
## master #1900 +/- ##
=======================================
Coverage 87.08% 87.08%
=======================================
Files 223 223
Lines 39847 39847
Branches 5169 5169
=======================================
Hits 34702 34702
Misses 4568 4568
Partials 577 577 |
| @@ -0,0 +1,453 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #9. pdx2 = p.dx2+p.dx.dzeta*zeta_dx+(p.dx.dzeta+p.dzeta2*zeta_dx)*zeta_dx+p.dzeta*zeta_dxx
Would be better to use a .subs() here and substitute into the standard equation (better for complex transforms)
Reply via ReviewNB
| @@ -0,0 +1,453 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #18. for i in range(f.space_order//2):
Use Mathias' free-surface with a SubDomain
Reply via ReviewNB
EdCaunt
force-pushed
the
variable_z_notebook
branch
from
c67142c
to
19e53be
Compare
| @@ -0,0 +1,549 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #5. def zeta_derivs(lambdify=False):
Maybe split this into several smaller functions that are easily relatable to the mathematical explanation that should be added in the intro?
Reply via ReviewNB
| @@ -0,0 +1,549 @@ | |||
| { | |||
There was a problem hiding this comment.
Should explain why this is the case. Also, may be worth referencing the subdomains notebook here.
Reply via ReviewNB
| @@ -0,0 +1,549 @@ | |||
| { | |||
There was a problem hiding this comment.
If the mathematics is added up top should be able to express this in a clearer manner.
Reply via ReviewNB
|
Overall looks good. Just added some comments above that I think would help make it more easily digestible. |
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "# Variable-z curvilinear topography\n", |
There was a problem hiding this comment.
Hmm, a comment didn't seem to appear in my earlier review?
In any case, I think the intro needs to go through the mathematics of what is being done here in a nice clear manner. The code below can then be related back to this as appropriate.
| @@ -0,0 +1,549 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #3. s_o = 8 # Space order
For homgeneity to the rest of the codebase I would use so
Reply via ReviewNB
| @@ -0,0 +1,549 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,549 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,549 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #15. src.coordinates.data[0, -1] = 3*grid.extent[1]/4 # Set depth
Maybe you can explcitly set the depth here, instead of having (3/4)*
Reply via ReviewNB
There was a problem hiding this comment.
I did it this way since the source is vertically positioned in the iteration space, rather than the physical space. Eventually it would be nice for sources to respect coordinate transforms so that one can position them in physical space on a curvilinear grid however
EdCaunt
force-pushed
the
variable_z_notebook
branch
from
19e53be
to
64e1d7d
Compare
I made a notebook with a simple implementation of a toy variable-z curvilinear acoustic model with a cosine hill. Still WIP, needs tidy-up and explanations, but lmk your thoughts