Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plasma Graph Tutorial #1925

Merged
merged 9 commits into from
Apr 7, 2022
Merged

Conversation

bartnikm
Copy link
Contributor

Description

This introduces a tutorial on how to plot the plasma graph within a jupyter notebook as well as saving it onto a .tex file

Motivation and context

There is no current tutorial on the graph, and since it has different methods of saving the graph, this aims to show examples of different ways the graph can be displayed.

How has this been tested?

  • Testing pipeline.
  • Other.

Examples

https://bartnikm.github.io/tardis-bartnikm/branch/graph-tutorial_doc/physics/setup/plasma/index.html#the-plasma-graph
https://bartnikm.github.io/tardis-bartnikm/branch/graph-tutorial_doc/physics/setup/plasma/graph_tutorial.html

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
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@tardis-bot
Copy link
Contributor

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.

@codecov
Copy link

codecov bot commented Mar 13, 2022

Codecov Report

Merging #1925 (67f7056) into master (5d6e229) will not change coverage.
The diff coverage is n/a.

❗ Current head 67f7056 differs from pull request most recent head c7a3a10. Consider uploading reports for the commit c7a3a10 to get more accurate results

@@           Coverage Diff           @@
##           master    #1925   +/-   ##
=======================================
  Coverage   58.76%   58.76%           
=======================================
  Files          70       70           
  Lines        7843     7843           
=======================================
  Hits         4609     4609           
  Misses       3234     3234           
Impacted Files Coverage Δ
...dis/montecarlo/montecarlo_numba/formal_integral.py 51.20% <0.00%> (-0.17%) ⬇️
tardis/tardis/plasma/properties/atomic.py 53.12% <0.00%> (ø)
tardis/tardis/io/atom_data/base.py 89.77% <0.00%> (+0.05%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@andrewfullard
Copy link
Contributor

Great work! Can you make a more "human readable" set of examples for the LaTeX plasma graph? The text is generally extremely small.

@isaacgsmith
Copy link
Member

Really nice -- I will have to get to a more thorough review later.

Just a quick thing to think about. This may belong in the "additional outputs" part of the "input/output" section of the docs, and just linked to in the plasma physics section. This is for two reasons: First, thematically this is an output of the simulation and not a part of the physics. Second, this is an important tutorial and I believe it belongs on the list of tutorials, which compiles notebooks in the "input/output" section of the docs.

Also, perhaps a title like "Generating the Plasma Graph" would be better, especially if it is moved to where I recommend.

This is just my opinion and someone else may have a more convincing argument for the notebook staying where it is.

Copy link
Contributor

@andrewfullard andrewfullard left a comment

Choose a reason for hiding this comment

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

Looking much better, well done!

Copy link
Member

@isaacgsmith isaacgsmith left a comment

Choose a reason for hiding this comment

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

This is a great PR. I hate to be so nit-picky but these are my comments/suggestions:

  • In my opinion, the packages needed to generate the graph (i.e. networkx) should be added to the TARDIS environment.
  • Plasma_Graph_Default.tex appears to be generated by the notebook and thus should not be in the repository and should be added to .gitignore (there is a section for notebook-generated files). The other generated files should also be added to .gitignore.
  • All tardis_example.yml files should be symlinks of docs/tardis_example.yml.
  • Is there a particular reason you use ipython to display images in addition to markdown HTML? All the images are shown twice. I typically prefer to use just markdown HTML, which would let you get rid of all the iframe stuff (including the import statement).
  • This is a very small point, but for consistency we try to have all filenames be in snake case.
  • Also very small, you should delete the empty code cell at the bottom of the notebook.
  • Finally, take a look at my previous comment about the location of your notebook.

@bartnikm bartnikm force-pushed the graph-tutorial_doc branch from 76148e7 to c7a3a10 Compare March 28, 2022 18:46
Copy link
Member

@isaacgsmith isaacgsmith left a comment

Choose a reason for hiding this comment

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

Once you rebase and I check out the docs build I should be able to approve!
Edit: Sorry, I must have missed that you already did.

Copy link
Member

@isaacgsmith isaacgsmith left a comment

Choose a reason for hiding this comment

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

Awesome!

@wkerzendorf wkerzendorf merged commit 2e1cb75 into tardis-sn:master Apr 7, 2022
@bartnikm bartnikm deleted the graph-tutorial_doc branch April 8, 2022 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants