examples: Refresh several notebooks in examples/ #2542
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 #2542 +/- ##
==========================================
- Coverage 91.99% 91.98% -0.01%
==========================================
Files 244 244
Lines 47703 47709 +6
Branches 4196 4196
==========================================
+ Hits 43884 43887 +3
- Misses 3145 3150 +5
+ Partials 674 672 -2
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:
|
georgebisbas
force-pushed
the
refresh_snapshotting
branch
from
dc954ee to
64baa3f
Compare
|
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:
georgebisbas commented on 2025-02-21T15:41:24Z That is a very valid answer. It should/could have been up to 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 / edit / reply to this conversation on ReviewNB JDBetteridge commented on 2025-02-20T12:09:12Z Line #2. factor = round(nt / nsnaps) # Save every This is the wrong thing to do in general (it might work here, but won't work for other values) see: 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 |
|
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 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 |
|
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 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 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 |
|
dropped View entire conversation on ReviewNB |
|
yes, thanks View entire conversation on ReviewNB |
|
That is a very valid answer. It should/could have been up to 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 |
|
Thanks! View entire conversation on ReviewNB |
|
Right! View entire conversation on ReviewNB |
|
Yes View entire conversation on ReviewNB |
|
it will get out of bounds with ceil View entire conversation on ReviewNB |
|
yes View entire conversation on ReviewNB |
|
yes View entire conversation on ReviewNB |
|
I think this helps in better following the example and code, with a small price of duplicate code View entire conversation on ReviewNB |
|
It helps readers to also understand how to save, they may want to other things with that View entire conversation on ReviewNB |
|
Yes View entire conversation on ReviewNB |
|
Wouldn't it be the other way around? Rounding the factor upward corresponds to rounding the number of snaps made downward. View entire conversation on ReviewNB |
|
Yes, but floor-ing nsnaps is closer in understanding what is the limiting factor, while the other one "deserves" an extra division in "thinking why". At least from my personal POV. View entire conversation on ReviewNB |
This reverts commit 3cbeb2d.
georgebisbas
force-pushed
the
refresh_snapshotting
branch
from
69fccc6 to
1091ee7
Compare
|
People are going to lift this code into their own code on the assumption it is correct. Using floor here can result in a segfault when running the kernel (since it can result in more snapshots than slots in which to store them) and is generally unsafe, whilst ceil is always safe. I would also add a comment explaining this View entire conversation on ReviewNB |
|
not needed, dropping View entire conversation on ReviewNB |
|
Yes View entire conversation on ReviewNB |
|
done View entire conversation on ReviewNB |
|
So, you agree with Jack above? As in I should use round? View entire conversation on ReviewNB |
|
Jack also said it should use ceil View entire conversation on ReviewNB |
No description provided.