Adding Convergence Plots #1636

atharva-2001 · 2021-06-08T05:27:35Z

This PR adds convergence plots to TARDIS.

Description

Convergence plots will allow the user to see the convergence of the simulation in plots and study the state of the simulation at different iterations. Convergence plots are plotted by default as the simulation runs. One can configure the plots by providing arguments in the run_tardis function. This includes overriding the default plotly layout of the graphs and options to change color scale and show or not show the plots.

Motivation and context

How has this been tested?

Testing pipeline.
Other.

Examples

Link to the convergence plots in the documentation.
Link to the notebook.
Link to the API

Convergence Plots along with progress bars

See also: #1629

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.

adding convergence plot notebook and python file

review-notebook-app · 2021-06-08T05:27:39Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

tardis-bot · 2021-06-08T05:50:41Z

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.

MarkMageeAstro · 2021-06-08T12:38:25Z

This looks like a great start. I have a few stylistic comments:

Plasma updates

Can you swap the plot round so that temperature is on the left? There are no labels for temperature or velocity, is this the latex rendering issue?
I think the first example you have with iterations and then just the numbers looks the best. It's hard to tell exactly what's going on in the last one because the lines aren't connected and that also makes it difficult to see the colours on the right hand side
It would be best to change the display formatting and add units. The full temperature should be printed out, not just rounded to the nearest 1000. For example, use 10,000 K and not 10k K. Similarly, the x-axis should be in km/s but at the minute is in cm/s
Personally, I think a different colour scheme would be good, like rainbow or jet. If we stick with this one then I would at least swap it round so that the lighter yellow is the last iteration.

Temperature plots

We need units here too. Luminosity should be in erg / s
A bar chart isn't really the best way to present this data, so I would remove it. The line plot is much better. It should also have a legend to say what each colour is and to say what the horizontal line is. I would use some more contrasting colours as well
I think the labels are the wrong way round. In the line plot, the final emitted luminosity is listed as 4e42, but the simulation output says this is the luminosity absorbed.
Might also be nice to show a residual difference between the emitted and requested luminosities for each iteration. This can just be the difference between the two given as a percentage of the luminosity requested or something like that. Could be a subpanel to the main plot, just below

fixing swapped y-axis labels in plasma plot

codecov · 2021-06-16T13:43:18Z

Codecov Report

Merging #1636 (2b08432) into master (2010e81) will increase coverage by 0.56%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1636      +/-   ##
==========================================
+ Coverage   61.88%   62.45%   +0.56%     
==========================================
  Files          63       64       +1     
  Lines        5851     5985     +134     
==========================================
+ Hits         3621     3738     +117     
- Misses       2230     2247      +17

Impacted Files	Coverage Δ
tardis/tardis/base.py	`57.14% <0.00%> (-0.55%)`	⬇️
tardis/tardis/visualization/widgets/shell_info.py	`95.95% <0.00%> (-0.05%)`	⬇️
...dis/tardis/visualization/tools/convergence_plot.py	`88.67% <0.00%> (ø)`
tardis/tardis/simulation/base.py	`83.17% <0.00%> (+0.28%)`	⬆️

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 2010e81...2b08432. Read the comment docs.

tardis/visualization/tools/convergence_plot.py

docs/io/visualization/convergence_plot.ipynb

tardis/visualization/tools/convergence_plot.py

jamesgillanders

This all looks great @atharva-2001! All requested changes seem to have been addressed, and the code itself works really well. Good job!

jaladh-singhal

Brilliant work! Thank you for addressing all of my nitpicky comments 😄

jaladh-singhal · 2021-07-22T19:59:59Z

@atharva-2001 this needs rebasing now that #1723 is merged

* initial commit adding convergence plot notebook and python file * updating class structure * adding functions to create empty plots * adding functions to update convergence plots * fixing imports for convergence plots * changes to get update convergence plots from tardis/simulation/base.py * adding updated notebook * allowing user to customize plot layout from run_tardis * adding options to not show plots and change colorscale * adding docstrings * adding updated notebook * fix typo fixing swapped y-axis labels in plasma plot * exporting convergence plots * layout changes fixing colors, removing marker points, fixing labels * adding check to see if data is collected necessary when running simulation with just one iteration * moving convergence plots notebook * adding tests for convergence class and transition_colors function * fix typos and adding comments * reformatted using black * adding function to override default plot configuration * showing how plots can be updated in the notebook * fixing plot heights and tick labels * code refactor raising TypeErrors, fix typos * adding docstrings * fixing axes and legend, converting units using astropy * fixing hover data and making axes titles romanized/upright in certain places * use same colorscale for both plasma and luminosity plots * add docstrings and code comments * add option to change colorscale, format using black, add updated notebook * remove unnecessary customizations, edit docstrings * add documentation in the notebook * test length of fig.data tuple after build * add tests for update and override function * edit docstrings and code comments * renaming luminosity_plot to t_inner_luminosities_plot * minor changes in documentation and docstrings * [build docs]

initial commit

00971e1

adding convergence plot notebook and python file

atharva-2001 marked this pull request as draft June 8, 2021 05:28

atharva-2001 added feature visualization work in progress 🚧 labels Jun 8, 2021

atharva-2001 requested review from jaladh-singhal, jamesgillanders and MarkMageeAstro June 8, 2021 05:50

atharva-2001 requested a review from marxwillia June 9, 2021 09:39

atharva-2001 added 3 commits June 14, 2021 18:14

updating class structure

94d6241

adding functions to create empty plots

af99842

adding functions to update convergence plots

5989612

atharva-2001 force-pushed the convergence_plots branch from 8dcb18d to 5989612 Compare June 14, 2021 16:04

atharva-2001 added 4 commits June 15, 2021 06:55

fixing imports for convergence plots

563e2c6

changes to get update convergence plots from tardis/simulation/base.py

75b593a

adding updated notebook

c712b26

allowing user to customize plot layout from run_tardis

f1f52ca

atharva-2001 force-pushed the convergence_plots branch from 3cb95ee to f1f52ca Compare June 15, 2021 12:55

adding options to not show plots and change colorscale

c64d195

atharva-2001 marked this pull request as ready for review June 15, 2021 14:33

adding docstrings

e70c266

atharva-2001 force-pushed the convergence_plots branch from 6e987a8 to e70c266 Compare June 15, 2021 15:17

atharva-2001 added 2 commits June 15, 2021 21:23

adding updated notebook

0c40895

fix typo

453e08b

fixing swapped y-axis labels in plasma plot

atharva-2001 force-pushed the convergence_plots branch from ff0085b to 453e08b Compare June 15, 2021 16:34

tardis-sn deleted a comment from tardis-bot Jun 17, 2021

jaladh-singhal reviewed Jul 15, 2021

View reviewed changes

tardis/visualization/tools/convergence_plot.py Show resolved Hide resolved

atharva-2001 mentioned this pull request Jul 15, 2021

Edit docstring in run_tardis #1723

Merged

10 tasks

jaladh-singhal reviewed Jul 15, 2021

View reviewed changes

docs/io/visualization/convergence_plot.ipynb Show resolved Hide resolved

docs/io/visualization/convergence_plot.ipynb Show resolved Hide resolved

docs/io/visualization/convergence_plot.ipynb Show resolved Hide resolved

atharva-2001 force-pushed the convergence_plots branch from 479405e to 3e98f7f Compare July 16, 2021 08:11

atharva-2001 requested a review from jaladh-singhal July 16, 2021 09:02

jaladh-singhal removed the work in progress 🚧 label Jul 16, 2021

jaladh-singhal reviewed Jul 16, 2021

View reviewed changes

tardis/visualization/tools/convergence_plot.py Outdated Show resolved Hide resolved

jamesgillanders approved these changes Jul 16, 2021

View reviewed changes

atharva-2001 force-pushed the convergence_plots branch 4 times, most recently from 746e2f9 to 69c9a8c Compare July 16, 2021 15:07

atharva-2001 requested a review from jaladh-singhal July 16, 2021 15:37

minor changes in documentation and docstrings

f624afc

atharva-2001 force-pushed the convergence_plots branch 4 times, most recently from 126fa86 to dab2341 Compare July 21, 2021 15:50

marxwillia approved these changes Jul 22, 2021

View reviewed changes

jaladh-singhal approved these changes Jul 22, 2021

View reviewed changes

Merge branch 'master' into convergence_plots

35c5693

atharva-2001 force-pushed the convergence_plots branch from dab2341 to 5baed30 Compare July 23, 2021 07:20

[build docs]

2b08432

atharva-2001 force-pushed the convergence_plots branch from 5baed30 to 2b08432 Compare July 23, 2021 07:23

marxwillia merged commit f27fe3d into tardis-sn:master Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Convergence Plots #1636

Adding Convergence Plots #1636

atharva-2001 commented Jun 8, 2021 •

edited

Loading

review-notebook-app bot commented Jun 8, 2021

tardis-bot commented Jun 8, 2021

MarkMageeAstro commented Jun 8, 2021

codecov bot commented Jun 16, 2021 •

edited

Loading

jamesgillanders left a comment

jaladh-singhal left a comment

jaladh-singhal commented Jul 22, 2021

Adding Convergence Plots #1636

Adding Convergence Plots #1636

Conversation

atharva-2001 commented Jun 8, 2021 • edited Loading

review-notebook-app bot commented Jun 8, 2021

tardis-bot commented Jun 8, 2021

MarkMageeAstro commented Jun 8, 2021

codecov bot commented Jun 16, 2021 • edited Loading

Codecov Report

jamesgillanders left a comment

Choose a reason for hiding this comment

jaladh-singhal left a comment

Choose a reason for hiding this comment

jaladh-singhal commented Jul 22, 2021

atharva-2001 commented Jun 8, 2021 •

edited

Loading

codecov bot commented Jun 16, 2021 •

edited

Loading