Making Changes to Several Tutorials #1763

isaacgsmith · 2021-07-29T20:00:16Z

Description

Changes include:

Some general formatting of notebooks.
Adding the download_atom_data function to every notebook that needs it so that the notebooks will run properly in binder.
- In doing this, I fixed the print statement in tardis/io/atom_data/util.py to use the correct import.
Addressing errors in all tutorials that had them (except for docs/io/configuration/components/models/Custom_TARDIS_Model_Tutorial.ipynb which is supposed to have those error -- I will be returning to this notebook in a later PR).
Clearing the output in all tutorials (besides docs/io/configuration/components/models/Custom_TARDIS_Model_Tutorial.ipynb) so that they will be executed by nbsphinx.
Adding in a small HDF to docs/io/visualization/ so that the visualization notebooks can run. Because I cleared the output in docs/io/visualization/sdec_plot.ipynb, this PR will still decrease the overall size of the repo.
Removing some of the plotly plots from docs/io/visualization/sdec_plot.ipynb so that the built file is <100 MB (GH pages requirement).
Deleting a duplicate notebook (docs/io/configuration/components/models/densitycust/Custom_Density_And_Boundary_Velocities.ipynb).
Adding a note to the quickstart guide.
Rewriting docs/io/optional/callback_example.ipynb.
Adding more text to docs/io/configuration/read_configuration.ipynb and including information about reading configuration dictionaries.
Moving docs/io/output/hdf_writer.ipynb to be under docs/outdated, allowing me to make our section about writing a simulation to an HDF much more simple.

NOTE: the docs building now takes way too long. I am exploring options to fix this.

Motivation and context

Now, nearly all of the TARDIS tutorials we want to work are actually working and formatted properly.

How has this been tested?

Testing pipeline.
Docs built locally and on GitHub.

Examples

https://smithis7.github.io/tardis/branch/notebook_format_doc/

Type of change

Bug fix.
New feature.
Breaking change.
None of the above.

Checklist

My change requires a change to the documentation.
- I have updated the documentation accordingly.
- (optional) I have built the documentation on my fork following the instructions.
I have assigned and requested two reviewers for this pull request.

review-notebook-app · 2021-07-29T20:00:24Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2021-07-29T20:11:22Z

Codecov Report

Merging #1763 (255d67b) into master (058adb5) will not change coverage.
The diff coverage is n/a.

❗ Current head 255d67b differs from pull request most recent head b6668f5. Consider uploading reports for the commit b6668f5 to get more accurate results

@@           Coverage Diff           @@
##           master    #1763   +/-   ##
=======================================
  Coverage   58.42%   58.42%           
=======================================
  Files          66       66           
  Lines        6684     6684           
=======================================
  Hits         3905     3905           
  Misses       2779     2779

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 058adb5...b6668f5. Read the comment docs.

tardis-bot · 2021-08-02T17:52:27Z

Before a pull request is accepted, it must meet the following criteria:

Is the necessary information provided?
Is this a duplicate PR?
- If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
- If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
Does it pass existing tests and are new tests provided if required?
- The test coverage should not decrease, and for new features should be close to 100%.
Is the code tidy?
- No unnecessary print lines or code comments.

docs/quickstart/quickstart.ipynb

docs/outdated/hdf_writer.ipynb

docs/io/configuration/read_configuration.ipynb

andrewfullard · 2021-08-04T15:36:30Z

I think we definitely need to be careful about removing information (e.g. the SDEC plot examples) but otherwise generally good

jaladh-singhal · 2021-08-05T19:09:57Z

docs/io/visualization/generating_widgets.ipynb

@@ -14,384 +14,28 @@
   "cell_type": "markdown",


Nice note at bottom :)

Reply via ReviewNB

jaladh-singhal · 2021-08-05T19:09:57Z

docs/quickstart/quickstart.ipynb

@@ -2,14 +2,13 @@
 "cells": [


I think @wkerzendorf mentioned to show INFO level logs!

Reply via ReviewNB

My thinking is that we want to show the defaults in quickstart. @wkerzendorf to my memory talked about changing it so "info" is the default.

jaladh-singhal

These changes are so thoughtful - awesome work @smithis7 🎉

There are few things:

Can you rename "TARDIS Grid Tutorial" to "TARDIS Grid" - I think tutorial looks bit odd considering how its sibling sections are named
It needs rebase, there are merge conflicts in logging notebook
Your created hdf don't show up Ar for real packet population but shows up for virtual - can you please fix that we really want to show same colorscale in both packets_mode to avoid confusion
About adding hdf to visualization folder seems reasonable to make it run on binder and since you've freed lot of space by removing outputs. But I'd ask @wkerzendorf to have a final say on this!

isaacgsmith · 2021-08-30T19:19:22Z

These changes are so thoughtful - awesome work @smithis7 🎉

There are few things:

Can you rename "TARDIS Grid Tutorial" to "TARDIS Grid" - I think tutorial looks bit odd considering how its sibling sections are named

It needs rebase, there are merge conflicts in logging notebook

Your created hdf don't show up Ar for real packet population but shows up for virtual - can you please fix that we really want to show same colorscale in both packets_mode to avoid confusion

About adding hdf to visualization folder seems reasonable to make it run on binder and since you've freed lot of space by removing outputs. But I'd ask @wkerzendorf to have a final say on this!

I have rebased and fixed the hdf issue. And I did talk to Wolfgang about adding the HDF before I submitted this PR.

For the "TARDIS Grid" page, there are a few things on that that are being talked about (i.e. the filename will be changed and we may move the file and possibly come up with a different name), so I think we should make any changes to that page all at once. And, Marc will make the changes to the file name so it still shows him as the page author.

jaladh-singhal

Sorry, it took me so long to review this - well done!

* changing hdf notebooks * formatting physical_quantities.ipynb * updating atom data error message * formatting read_configuration notebook * deleting duplicate notebook * actually deleting duplicate notebook * fixing visualization notebooks that require hdf * shrinking hdf * clearning notebook outputs * removing some sdec examples * small changes in many tutorials * redoing callback example and adding note to quickstart * a few changes to stella_to_tardis * adding atomic data to grid tutorial * clearing output in stella_to_tardis * adding missing import statement * fixing typos * changing hdf so that Ar shows in real SDEC plot

isaacgsmith added the documentation label Jul 29, 2021

isaacgsmith force-pushed the notebook_format_doc branch 2 times, most recently from 93da632 to 749ce23 Compare August 2, 2021 16:02

isaacgsmith changed the title ~~Notebook format doc~~ Making Changes to several tutorials Aug 2, 2021

isaacgsmith changed the title ~~Making Changes to several tutorials~~ Making Changes to Several Tutorials Aug 2, 2021

isaacgsmith requested review from DhruvSondhi, andrewfullard and jaladh-singhal August 2, 2021 17:52

isaacgsmith marked this pull request as ready for review August 2, 2021 17:53

andrewfullard reviewed Aug 4, 2021

View reviewed changes

docs/quickstart/quickstart.ipynb Outdated Show resolved Hide resolved

andrewfullard reviewed Aug 4, 2021

View reviewed changes

docs/outdated/hdf_writer.ipynb Show resolved Hide resolved

andrewfullard reviewed Aug 4, 2021

View reviewed changes

docs/io/configuration/read_configuration.ipynb Show resolved Hide resolved

tardis-sn deleted a comment from tardis-bot Aug 4, 2021

isaacgsmith requested a review from andrewfullard August 4, 2021 18:46

andrewfullard approved these changes Aug 5, 2021

View reviewed changes

jaladh-singhal reviewed Aug 5, 2021

View reviewed changes

andrewfullard marked this pull request as draft August 6, 2021 13:10

isaacgsmith marked this pull request as ready for review August 19, 2021 20:08

isaacgsmith added 7 commits August 30, 2021 14:51

changing hdf notebooks

a6388e2

formatting physical_quantities.ipynb

0d39bd0

updating atom data error message

8ddde2d

formatting read_configuration notebook

04177ca

deleting duplicate notebook

aef365c

actually deleting duplicate notebook

a1ddd84

fixing visualization notebooks that require hdf

3193f23

isaacgsmith added 11 commits August 30, 2021 14:51

shrinking hdf

b41a3ca

clearning notebook outputs

3eceeaa

removing some sdec examples

6607945

small changes in many tutorials

0f22d13

redoing callback example and adding note to quickstart

dcf2272

a few changes to stella_to_tardis

50bafb7

adding atomic data to grid tutorial

a6e66ea

clearing output in stella_to_tardis

0e9bfb4

adding missing import statement

12b20ab

fixing typos

d095796

changing hdf so that Ar shows in real SDEC plot

b6668f5

isaacgsmith force-pushed the notebook_format_doc branch from 27b8f82 to b6668f5 Compare August 30, 2021 19:14

isaacgsmith requested a review from jaladh-singhal August 30, 2021 19:20

isaacgsmith linked an issue Sep 2, 2021 that may be closed by this pull request

Visualization notebooks are pre-run #1756

Closed

jaladh-singhal approved these changes Sep 17, 2021

View reviewed changes

jaladh-singhal merged commit b579fc1 into tardis-sn:master Sep 17, 2021

isaacgsmith deleted the notebook_format_doc branch October 4, 2021 18:37

atharva-2001 mentioned this pull request Feb 9, 2022

Outdated code in to_hdf notebook #1669

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making Changes to Several Tutorials #1763

Making Changes to Several Tutorials #1763

isaacgsmith commented Jul 29, 2021 •

edited

Loading

review-notebook-app bot commented Jul 29, 2021

codecov bot commented Jul 29, 2021 •

edited

Loading

tardis-bot commented Aug 2, 2021

andrewfullard commented Aug 4, 2021

jaladh-singhal Aug 5, 2021

jaladh-singhal Aug 5, 2021

isaacgsmith Aug 6, 2021

jaladh-singhal left a comment •

edited

Loading

isaacgsmith commented Aug 30, 2021

jaladh-singhal left a comment

Making Changes to Several Tutorials #1763

Making Changes to Several Tutorials #1763

Conversation

isaacgsmith commented Jul 29, 2021 • edited Loading

review-notebook-app bot commented Jul 29, 2021

codecov bot commented Jul 29, 2021 • edited Loading

Codecov Report

tardis-bot commented Aug 2, 2021

andrewfullard commented Aug 4, 2021

jaladh-singhal Aug 5, 2021

Choose a reason for hiding this comment

jaladh-singhal Aug 5, 2021

Choose a reason for hiding this comment

isaacgsmith Aug 6, 2021

Choose a reason for hiding this comment

jaladh-singhal left a comment • edited Loading

Choose a reason for hiding this comment

isaacgsmith commented Aug 30, 2021

jaladh-singhal left a comment

Choose a reason for hiding this comment

isaacgsmith commented Jul 29, 2021 •

edited

Loading

codecov bot commented Jul 29, 2021 •

edited

Loading

jaladh-singhal left a comment •

edited

Loading