FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

examples: Dispersion relation preserving notebook by JDBetteridge · Pull Request #2595 · devitocodes/devito · GitHub

/ devito Public

examples: Dispersion relation preserving notebook - #2595

Merged
mloubout merged 8 commits into
mainfrom
JDBetteridge/dispersion_notebook
Sep 10, 2025
ConversationCommits (8)ChecksFiles changed
Merged

examples: Dispersion relation preserving notebook#2595
mloubout merged 8 commits into
mainfrom
JDBetteridge/dispersion_notebook

Conversation

JDBetteridge commented May 1, 2025

Copy link
Copy Markdown
Contributor

No description provided.

review-notebook-app Bot commented May 1, 2025

Copy link
Copy Markdown

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov Bot commented May 1, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.14%. Comparing base (5111506) to head (4809a0e).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2595   +/-   ##
=======================================
  Coverage   92.14%   92.14%           
=======================================
  Files         248      248           
  Lines       50022    50022           
  Branches     4402     4402           
=======================================
  Hits        46091    46091           
  Misses       3222     3222           
  Partials      709      709
Flag Coverage Δ
pytest-gpu-aomp-amdgpuX 70.90% <ø> (+0.01%) ⬆️
pytest-gpu-nvc-nvidiaX 71.87% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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 examples.seismic.source.py that whole file could be 5 lines IMHO. I will change it if I'm instructed to, but this is much clearer

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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!

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

georgebisbas commented on 2025-05-02T06:30:14Z
----------------------------------------------------------------

Courant*

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

georgebisbas commented on 2025-05-02T06:30:16Z
----------------------------------------------------------------

velocity*

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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* 😂

FabioLuporini requested review from EdCaunt and mloubout May 2, 2025 07:16
JDBetteridge requested a review from ggorman May 2, 2025 08:53

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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


JDBetteridge commented on 2025-09-10T00:14:34Z
----------------------------------------------------------------

This is going to be addressed in a refactor in a separate PR

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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 plot not bar


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

mloubout commented on 2025-08-05T12:43:38Z
----------------------------------------------------------------

It's not clearer, people, especially the target audience, do not look at the Fourier spectrum like that, but as line plots. This makes it confusing. It's commonplace for a reason.

JDBetteridge commented on 2025-09-10T00:14:55Z
----------------------------------------------------------------

Fixed

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

mloubout commented on 2025-05-02T12:33:45Z
----------------------------------------------------------------

Maybe point to where it's computed in examples/seismic/model.py so user/reader see it's implemented in the examples.

Also maybe h_min in case someone use different spacing in different dimensions


review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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).

JDBetteridge commented May 2, 2025

Copy link
Copy Markdown
Contributor Author

Fixed

View entire conversation on ReviewNB

JDBetteridge commented May 2, 2025

Copy link
Copy Markdown
Contributor Author

Comment was superfluous anyway

View entire conversation on ReviewNB

JDBetteridge commented May 2, 2025

Copy link
Copy Markdown
Contributor Author

It isn't, but I really don't think it's necessary to invoke the wrath of examples.seismic.source.py that whole file could be 5 lines IMHO. I will change it if I'm instructed to, but this is much clearer

View entire conversation on ReviewNB

JDBetteridge commented May 2, 2025

Copy link
Copy Markdown
Contributor Author

Fixed

View entire conversation on ReviewNB

JDBetteridge commented May 2, 2025

Copy link
Copy Markdown
Contributor Author

Yes!

View entire conversation on ReviewNB

JDBetteridge commented May 2, 2025

Copy link
Copy Markdown
Contributor Author

Superfluous comment anyway

View entire conversation on ReviewNB

JDBetteridge commented May 2, 2025

Copy link
Copy Markdown
Contributor Author

Also see* 😂

View entire conversation on ReviewNB

EdCaunt commented May 2, 2025

Copy link
Copy Markdown
Contributor

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

review-notebook-app Bot commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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)

review-notebook-app Bot commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

mloubout commented on 2025-08-05T12:57:21Z
----------------------------------------------------------------

Would print the whole array weights to show it gives you all the orders


JDBetteridge commented on 2025-09-10T00:21:34Z
----------------------------------------------------------------

This is done above in section 1

review-notebook-app Bot commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

mloubout commented on 2025-08-05T12:57:21Z
----------------------------------------------------------------

Can you make the grid coarser, the r=0.336 still shows dispersion would be "nicer" to go from no dispersion to dispersion


JDBetteridge commented on 2025-09-10T00:22:57Z
----------------------------------------------------------------

I think this is sufficient to get the point across

review-notebook-app Bot commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

mloubout commented on 2025-08-05T12:57:22Z
----------------------------------------------------------------

It is not unstable; unstable is for dt> Courant. It is just very dispersive. You can also point out that from the dispersion curves, when it gets dispersive, the high frequencies travel faster, which explains the early arrival.


JDBetteridge commented on 2025-09-10T00:24:18Z
----------------------------------------------------------------

You're right it's not unstable for this choice of velocity, but it is for the whole range considered as outlined in the introduction. This is what the calculation is based on. I will add clarification

review-notebook-app Bot commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

mloubout commented on 2025-08-05T12:57:23Z
----------------------------------------------------------------

Label axis.

review-notebook-app Bot commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

mloubout commented on 2025-08-05T12:57:23Z
----------------------------------------------------------------

It's still slightly dispersive for r=0.336, would go better with the message to have at least one clean case without dispersion visible.


JDBetteridge commented on 2025-09-10T00:34:15Z
----------------------------------------------------------------

We see that in the final example below

review-notebook-app Bot commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

mloubout commented on 2025-08-05T12:57:24Z
----------------------------------------------------------------

Maybe add some markers so can see both on the left


JDBetteridge commented on 2025-09-10T00:37:08Z
----------------------------------------------------------------

I made it dots

EdCaunt approved these changes Aug 5, 2025
View reviewed changes

EdCaunt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think I'm pretty happy with this now

JDBetteridge force-pushed the JDBetteridge/dispersion_notebook branch from 323da99 to a9584f6 Compare August 12, 2025 23:25

JDBetteridge commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

This adds widgets, they used to be part of ipython or matplotlib or notebook, but have been factored into their own package

View entire conversation on ReviewNB

JDBetteridge commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

This is going to be addressed in a refactor in a separate PR

View entire conversation on ReviewNB

JDBetteridge commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

Fixed

View entire conversation on ReviewNB

JDBetteridge commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

I was referencing that whole section, but it's fixed now

View entire conversation on ReviewNB

JDBetteridge commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

They are optimal for accuracy according to the reference:

of optimal formal order of accuracy

View entire conversation on ReviewNB

JDBetteridge commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

This is done above in section 1

View entire conversation on ReviewNB

JDBetteridge commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

I think this is sufficient to get the point across

View entire conversation on ReviewNB

JDBetteridge commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

You're right it's not unstable for this choice of velocity, but it is for the whole range considered as outlined in the introduction. This is what the calculation is based on. I will add clarification

View entire conversation on ReviewNB

JDBetteridge commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

We see that in the final example below

View entire conversation on ReviewNB

JDBetteridge commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

I made it dots

View entire conversation on ReviewNB

JDBetteridge force-pushed the JDBetteridge/dispersion_notebook branch from a9584f6 to 4809a0e Compare September 10, 2025 00:41
mloubout merged commit 91e2f1d into main Sep 10, 2025
36 checks passed
mloubout deleted the JDBetteridge/dispersion_notebook branch September 10, 2025 02:45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mloubout mloubout approved these changes
EdCaunt EdCaunt approved these changes
ggorman Awaiting requested review from ggorman

Assignees

No one assigned

Labels

documentation examples examples

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL