examples: Refresh examples/seismic/tutorials/08_snapshotting.ipynb

[georgebisbas]

No description provided.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.28%. Comparing base (34dba05) to head (88b8a66).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2542      +/-   ##
==========================================
- Coverage   87.29%   87.28%   -0.01%     
==========================================
  Files         238      238              
  Lines       46063    46063              
  Branches     4080     4080              
==========================================
- Hits        40211    40208       -3     
- Misses       5162     5163       +1     
- Partials      690      692       +2     

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

[mloubout]

Looks fine to me.

Just a generic request that has nothing to do with the changes, can we please use higher order (like 4 or 8), the wavefield do not look nice at all there is too much dispersion with space order 2

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:09Z
----------------------------------------------------------------

Line #50.        time_range=time_range)  # new

What is this comment?

georgebisbas commented on 2025-02-21T15:33:16Z
----------------------------------------------------------------

hm...did not notice, some leftover from the original contributor I guess

georgebisbas commented on 2025-02-21T15:33:36Z
----------------------------------------------------------------

dropped

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:10Z
----------------------------------------------------------------

Line #51.    rec.coordinates.data[:, 0] = np.linspace(0, model.domain_size[0], num=11)

nrecs?

georgebisbas commented on 2025-02-21T15:33:38Z
----------------------------------------------------------------

yes, thanks

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:11Z
----------------------------------------------------------------

Line #76.    op(time=time_range.num-2, dt=model.critical_dt)

This has been a point of confusion for me:

• Why is time = time_range.num - 2?
• How does the user know this?
• And also does it even need to be specified in this case?

georgebisbas commented on 2025-02-21T15:41:24Z
----------------------------------------------------------------

That is a very valid answer. It should/could have been up to time_range.num - 1,

but this is needed cuz of this bug here: #2235

which I attempted to solve here: #2237

On the other hand Devito is smart enough to run up to this point, so indeed it does not need to be specified.

So I can safely drop I think.

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:12Z
----------------------------------------------------------------

Line #2.    factor = round(nt / nsnaps)  # Save every factor nsnaps, for any nt

This is the wrong thing to do in general (it might work here, but won't work for other values) see:

devito/examples/userapi/07_functions_on_subdomains.ipynb

Line 1076 in 29456c6

"factor = int(np.ceil(nt/nsnaps))\n",

georgebisbas commented on 2025-02-21T15:45:23Z
----------------------------------------------------------------

Thanks!

georgebisbas commented on 2025-02-21T15:50:21Z
----------------------------------------------------------------

Though we need floor here not ceil

JDBetteridge commented on 2025-02-21T18:14:40Z
----------------------------------------------------------------

If you use floor, you may end up writing more than nsnaps snaps, ie more than you have allocated for. This was the issue Ed was seeing in his notebook and the generated code would (attempt to) write beyond the end of the allocated array. It's counter-intuitive TBH, but that's a more general issue!

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:13Z
----------------------------------------------------------------

Line #2.    plt.rcParams['figure.figsize'] = (20, 20)  # Increases figure size

This seems wrong, if you're going to change the rcParams do it once at the top of the notebook, otherwise use plt.figure(figsize=(w, h)) and choose an appropriate width and height

georgebisbas commented on 2025-02-21T15:47:18Z
----------------------------------------------------------------

Right!

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:13Z
----------------------------------------------------------------

Line #5.    indices = np.linspace(0, nsnaps-1, plot_num, dtype=int)  # Indices for snapshots

You don't need this linspace just use an appropriate range

georgebisbas commented on 2025-02-21T15:52:21Z
----------------------------------------------------------------

Sure

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:14Z
----------------------------------------------------------------

Line #7.    plt.rcParams['figure.figsize'] = (20, 20) # Increases figure size

This is all copy pasta from above

georgebisbas commented on 2025-02-21T15:53:05Z
----------------------------------------------------------------

Yes

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:15Z
----------------------------------------------------------------

Line #4.    nsnaps = 100                 # desired number of equally spaced snaps

Is redefining this deliberate?

georgebisbas commented on 2025-02-21T16:23:27Z
----------------------------------------------------------------

The previous version was using some "magic numbers" only working for specific cases. I would like to keep the 100 snaps defined here for smoother transition

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:16Z
----------------------------------------------------------------

Line #5.    factor = round(nt / nsnaps)  # subsequent calculated factor

See above comment

georgebisbas commented on 2025-02-21T16:22:33Z
----------------------------------------------------------------

ok

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:16Z
----------------------------------------------------------------

Line #35.    op1(time=factor*nsnaps - 1, dt=model.critical_dt)  # run only for comparison

I thought that the point of the conditional dimension was that you could run to nt - 1 or nt -2 or whatever timesteps safely. If this isn't the case it could be the factor that needs adjusting

georgebisbas commented on 2025-02-21T16:25:22Z
----------------------------------------------------------------

This is rather related to the size of usave.The conditional Dimension knows only about doing usave=u every factor timesteps .

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:17Z
----------------------------------------------------------------

Line #7.    plt.rcParams['figure.figsize'] = (20, 20)  # Increases figure size

See above comments about this repeated code

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

JDBetteridge commented on 2025-02-20T12:09:18Z
----------------------------------------------------------------

Line #39.    # Adjust the size of the HTML video element to fit notebook width

I think the figure size needs adjusting here before you convert to HTML.

There is also an errant "> in the cell output

georgebisbas commented on 2025-02-21T16:34:56Z
----------------------------------------------------------------

hmmm... any help with that, what size should the figure be?

JDBetteridge commented on 2025-02-21T18:17:53Z
----------------------------------------------------------------

When I wrote my guide, I settled on (8,6), but this might need adjustment. If you are using plt.figure(figsize=(w, h)) you can try a few values. Given that this plot is a different aspect to the above plots, it might be a case against using the rc.params

JDBetteridge commented on 2025-02-21T18:19:29Z
----------------------------------------------------------------

Once upon a time I wrote a guide for matplotlib (in jupyter notebooks), you may or may not find it useful. It's still available here.

[JDBetteridge]

I've been a bit nitpicky with my comments, but as someone who's been reading the notebooks a lot recently there are often quite a few patterns that aren't completely obvious to me!

[georgebisbas]

hm...did not notice, some leftover from the original contributor I guess

---

View entire conversation on ReviewNB

[georgebisbas]

dropped

---

View entire conversation on ReviewNB

[georgebisbas]

yes, thanks

---

View entire conversation on ReviewNB

[georgebisbas]

That is a very valid answer. It should/could have been up to time_range.num - 1,

but this is needed cuz of this bug here: #2235

which I attempted to solve here: #2237

On the other hand Devito is smart enough to run up to this point, so indeed it does not need to be specified.

So I can safely drop I think.

---

View entire conversation on ReviewNB

[georgebisbas]

Thanks!

---

View entire conversation on ReviewNB

[georgebisbas]

Right!

---

View entire conversation on ReviewNB

[georgebisbas]

Though we need floor here not ceil

---

View entire conversation on ReviewNB

[georgebisbas]

Sure

---

View entire conversation on ReviewNB

[georgebisbas]

Yes

---

View entire conversation on ReviewNB

[georgebisbas]

ok

---

View entire conversation on ReviewNB

[georgebisbas]

The previous version was using some "magic numbers" only working for specific cases. I would like to keep the 100 snaps defined here for smoother transition

---

View entire conversation on ReviewNB

[georgebisbas]

This is rather related to the size of usave.The conditional Dimension knows only about doing usave=u every factor timesteps .

---

View entire conversation on ReviewNB

[georgebisbas]

hmmm... any help with that, what size should the figure be?

---

View entire conversation on ReviewNB

[JDBetteridge]

If you use floor, you may end up writing more nsnaps than you have allocated for. This was the issue Ed was seeing in his notebook and the generated code would (attempt to) write beyond the end of the allocated array. It's counter-intuitive TBH, but that's a more general issue!

---

View entire conversation on ReviewNB

[JDBetteridge]

When I wrote my guide, I settled on (8,6), but this might need adjustment. If you are using plt.figure(figsize=(w, h)) you can try a few values. Given that this plot is a different aspect to the above plots, it might be a case against using the rc.params

---

View entire conversation on ReviewNB

[JDBetteridge]

Once upon a time I wrote a guide for matplotlib (in jupyter notebooks), you may or may not find it useful. It's still available here.

---

View entire conversation on ReviewNB

[georgebisbas]

Thanks for that @JDBetteridge, I think the ratio in the screen for this video should be fine with the current dimension sizes.