devitocodes
examples: Refresh several notebooks in examples/
Check out this pull request on 
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
![review-notebook-app[bot] avatar](https://avatars.githubusercontent.com/in/15474?v=4 "Published by a pub.dev verified publisher"){kind=small}
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 91.32%. Comparing base (
b5b13e3) to head (016b17c).
Additional details and impacted files
@@ Coverage Diff @@
## main #2542 +/- ##
==========================================
- Coverage 91.47% 91.32% -0.16%
==========================================
Files 245 245
Lines 48802 48802
Branches 4307 4307
==========================================
- Hits 44640 44566 -74
- Misses 3461 3527 +66
- Partials 701 709 +8
| Flag | Coverage Δ | |
|---|---|---|
| pytest-gpu-aomp-amdgpuX | 72.58% <ø> (?) |
|
| pytest-gpu-nvc-nvidiaX | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
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
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
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: https://github.com/devitocodes/devito/issues/2235
which I attempted to solve here: https://github.com/devitocodes/devito/pull/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 / 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: https://github.com/devitocodes/devito/blob/29456c6465c58c1edb40c4962eae1847bd2f1e4e/examples/userapi/07_functions_on_subdomains.ipynb?short_path=d653ef1#L1076
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!
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!
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
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
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
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
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 .
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
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.
hm...did not notice, some leftover from the original contributor I guess
View entire conversation on ReviewNB
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: https://github.com/devitocodes/devito/issues/2235
which I attempted to solve here: https://github.com/devitocodes/devito/pull/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
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
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
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
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
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