examples: First complete draft of dispersion notebook #2595
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2595 +/- ##
==========================================
- Coverage 91.91% 91.29% -0.62%
==========================================
Files 245 245
Lines 48313 48495 +182
Branches 4244 4261 +17
==========================================
- Hits 44406 44273 -133
- Misses 3232 3522 +290
- Partials 675 700 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
View / edit / reply to this conversation on ReviewNB georgebisbas commented on 2025-05-02T06:30:10Z I think we usually have references at the last cell of a notebook. Not a strict rule though. JDBetteridge commented on 2025-05-02T12:36:06Z Fixed |
|
View / edit / reply to this conversation on ReviewNB georgebisbas commented on 2025-05-02T06:30:11Z Line #1. # We import everything we need for the noterbook at the start notebook typo JDBetteridge commented on 2025-05-02T12:36:08Z Comment was superfluous anyway |
|
View / edit / reply to this conversation on ReviewNB georgebisbas commented on 2025-05-02T06:30:12Z Should you add a comment on how it is (if it is) different from Deviot's builtin Ricker? JDBetteridge commented on 2025-05-02T12:36:09Z It isn't, but I really don't think it's necessary to invoke the wrath of |
|
View / edit / reply to this conversation on ReviewNB georgebisbas commented on 2025-05-02T06:30:12Z Citation can be a link I guess JDBetteridge commented on 2025-05-02T12:36:14Z Fixed |
|
View / edit / reply to this conversation on ReviewNB georgebisbas commented on 2025-05-02T06:30:13Z receiver* JDBetteridge commented on 2025-05-02T12:36:17Z Yes! |
|
View / edit / reply to this conversation on ReviewNB georgebisbas commented on 2025-05-02T06:30:14Z Courant* |
|
View / edit / reply to this conversation on ReviewNB georgebisbas commented on 2025-05-02T06:30:15Z Line #2. # space as per the work of Tam and Webb, Caunt Citations can be lifted to previous cell, and add link JDBetteridge commented on 2025-05-02T12:36:26Z Superfluous comment anyway |
|
View / edit / reply to this conversation on ReviewNB georgebisbas commented on 2025-05-02T06:30:16Z velocity* |
|
View / edit / reply to this conversation on ReviewNB georgebisbas commented on 2025-05-02T06:30:16Z Indistinguishable?* JDBetteridge commented on 2025-05-02T12:36:41Z Also see* 😂 |
|
View / edit / reply to this conversation on ReviewNB mloubout commented on 2025-05-02T12:33:42Z Nitpicking: We usually group the imports as
lib
external
devito
so here would be
from functools import partial import numpy as np import scipy as sp import sympy as sym from matplotlib import patheffects import matplotlib.colors as colors import matplotlib.pyplot as plt from devito import Grid, Function, TimeFunction, SparseTimeFunction, Eq, Operator, solve |
|
View / edit / reply to this conversation on ReviewNB mloubout commented on 2025-05-02T12:33:43Z THis should be available in examples/seismic/sources.py , maybe make it an importable function |
|
View / edit / reply to this conversation on ReviewNB mloubout commented on 2025-05-02T12:33:44Z Spectrum (Fourier transform) is usualy plotted as continuous in geophys, so just EdCaunt commented on 2025-06-02T11:01:47Z I quite like it as bars. It more clearly identifies the harmonics. I think conflating spectrum and spectral envelope is quite confusing really and probably bad practice, even if it is commonplace |
|
View / edit / reply to this conversation on ReviewNB mloubout commented on 2025-05-02T12:33:45Z Maybe point to where it's computed in
Also maybe |
|
View / edit / reply to this conversation on ReviewNB mloubout commented on 2025-05-02T12:33:45Z Never seen that left one, that's interesting. EdCaunt commented on 2025-05-02T14:10:45Z It corresponds to the "squaring" effect of numerical dispersion on an expanding wavefront (effective velocity is greater off-axis and less on-axis).
|
|
Fixed View entire conversation on ReviewNB |
|
Comment was superfluous anyway View entire conversation on ReviewNB |
|
It isn't, but I really don't think it's necessary to invoke the wrath of View entire conversation on ReviewNB |
|
Fixed View entire conversation on ReviewNB |
|
Yes! View entire conversation on ReviewNB |
|
Superfluous comment anyway View entire conversation on ReviewNB |
|
Also see* 😂 View entire conversation on ReviewNB |
|
It corresponds to the "squaring" effect of numerical dispersion on an expanding wavefront (effective velocity is greater off-axis and less on-axis).
View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:20:18Z Typo: "peeking" (to surreptitiously look at something), not "peaking" (to reach a highest point) |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:20:19Z Line #12. print(f'Optimal weights for {ii}{th} derivative on the {jj + ii + 1} point(s) x = {x[:jj + ii + 1]}:\n\t {w[:jj + ii + 1]}')
Nitpick: Split this print over two lines.
Have you flake8 linted the notebook with |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:20:20Z Nitpick: "Investigation of dispersion properties" would read more naturally
|
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:20:21Z I presume it assumes an antisymmetric stencil for first derivatives, or is this analysis not applicable? |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:20:21Z Typo: "missfit" -> "misfit"
Also this paragraph is pretty garbled and needs a rewrite |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:20:22Z This will need an |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:20:23Z "First consider the wavefield" would be more natural phrasing.
Typos: "dasahed" -> "dashed", "Courtant" -> "Courant"
"slightly sub-critical" would be better phrasing imo |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:46:27Z Typo: "correspinging" -> "corresponding" |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:46:28Z Typo: "dispersions" -> "dispersion" |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:46:29Z Typo: "trapesium" -> "trapezium" |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:46:29Z Probably reword |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:46:30Z "Choosing some objective function that minimises some measurable quantity of interest, in this case dispersion." is a little tautologous. Perhaps "devising measure of dispersion accuracy to suit the problem of interest"
|
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:46:31Z Typo: "grid pacing" |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:46:32Z Line #1. resolution = 101 The plots would look nicer if you doubled this and turned on |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:46:32Z Typo: "cross secion", "wave number" -> "wavenumber", "trapesium" -> "trapezium" |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:46:33Z Skip this cell otherwise it's going to massively slow down CI!
|
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-05-02T14:46:34Z Typo: "optimisaion" |
|
I quite like it as bars. It more clearly identifies the harmonics. I think conflating spectrum and spectral envelope is quite confusing really and probably bad practice, even if it is commonplace View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB EdCaunt commented on 2025-06-02T11:03:39Z Nitpick: This markdown cell can be consolidated with the one above |
No description provided.