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.

Sign up for GitHub

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

Jump to bottom

Convergence plots Demo [Do Not Merge] #1629

Closed
wants to merge 2,612 commits into from
Closed

Convergence plots Demo [Do Not Merge] #1629

wants to merge 2,612 commits into from

Conversation

atharva-2001
Copy link
Member

@atharva-2001 atharva-2001 commented Jun 7, 2021 •
edited
Loading

This PR aims to share a demo of convergence plots to be made for TARDIS.
Please see: #1636

Description

Motivation and context

How has this been tested?

  • Testing pipeline.
  • Other.

Examples
Link to view the combined plots as plotly subplots
Link to convergence plots with Vbox.
Link to the above convergence plot notebook in the documentation

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.

andrewfullard and others added 30 commits December 9, 2020 10:46
@andrewfullard
Fix test to match new style
1008b9b
@andrewfullard
Merge pull request tardis-sn#1354 from tardis-sn/virt-packet-logging-fix
7fa5d16 
Virtual packet logging fix
@andrewfullard
Fixed rpacket logging of properties
a49e4cc
@andrewfullard
Merge pull request tardis-sn#1375 from andrewfullard/rpacket-logging-fix
bb0a1b4 
Fixed real packet logging of properties
@epassaro
Update CI doc section (tardis-sn#1372)
da51efa 
* add some GA references

* more changes

* more changes
@andrewfullard
Merge pull request tardis-sn#1371 from smithis7/cleaning_docs
1c706d5 
Reorganizing files in docs (1/2)
@isaacgsmith
fixing mistake
5236056
@isaacgsmith
renaming running to using
1597b1c
@andrewfullard
Merge pull request tardis-sn#1378 from smithis7/fixing_index
6565a80 
Adding Plasma File in TOC
@isaacgsmith
fixing merge conflict
8c53894
@isaacgsmith
changing tardis_widgits to gui
3ac370b
@isaacgsmith
turning tardis_widgits into gui on docs/index.rst
002e635
@isaacgsmith
putting models in using/components
683d5d7
@isaacgsmith
adding configuration to docs/using/components
e358411
@isaacgsmith
adding atomic to docs/using/components
140d540
@isaacgsmith
fixing hyperlink in quickstart
9ffee87
@andrewfullard
FIx real packet logging for last interaction type
2def773
@andrewfullard
Virtual packet logging fix
613527a
@andrewfullard
Merge pull request tardis-sn#1379 from smithis7/reorganizing_2
ce5b665 
Reorganizing files in docs (2/2)
@isaacgsmith
fixing quickstart problem
3c2b83b
@andrewfullard
Merge pull request tardis-sn#1382 from smithis7/build_problem
162f280 
Fixing Quickstart Problem
@andrewfullard
Possible fix for vpacket last interaction spectrum
d4c74f1
@epassaro
Small fixes for existing workflows (tardis-sn#1384)
77c2782
@andrewfullard @jaladh-singhal
Update tardis/montecarlo/montecarlo_numba/single_packet_loop.py
1710023 
Co-authored-by: Jaladh Singhal <[email protected]>
@andrewfullard @jaladh-singhal
Update tardis/montecarlo/montecarlo_numba/single_packet_loop.py
4a509b6 
Co-authored-by: Jaladh Singhal <[email protected]>
@andrewfullard
Merge pull request tardis-sn#1380 from andrewfullard/packet-logging-o…
575c228 
…atch

Packet logging fixes for visualization
@jaladh-singhal
Added modified kromer plot to work with widgets
6ec4b8f 
Does not depend on minimal_model and contains data of both real & virtual mode
@jaladh-singhal
Converted SimulationKromerPlotter to class method
5b6a635
@jaladh-singhal
Added missing required properties to save in hdf
f7503ee
@jaladh-singhal
Added classmethod to create plotter from hdf
02f9500
@atharva-2001 atharva-2001 force-pushed the convergence_plots_demo branch from 7a0bcca to f4f77c2 Compare June 8, 2021 17:36
@atharva-2001 atharva-2001 requested a review from marxwillia June 9, 2021 09:39
@atharva-2001 atharva-2001 force-pushed the convergence_plots_demo branch 2 times, most recently from 863403b to 9050ea2 Compare June 9, 2021 15:52
@atharva-2001 atharva-2001 force-pushed the convergence_plots_demo branch from 9050ea2 to 1fc7f91 Compare June 9, 2021 15:54
jaladh-singhal
jaladh-singhal reviewed Jun 17, 2021
View reviewed changes
Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

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

The VBox solution looks great (moving iterations legends to above) but it affected the alignment of top & bottom plots' rightmost edge which was fine in combined plots. Maybe you can delve into CSS and reduce the width of above plot in a calculated way that alignment looks fine.

Also, I was thinking that VBox solution won't render in html notebooks, so sticking with combined plot (plotly sublot) will be more beneficial since it will render fine. Can you please search if plotly allows creating 2 legend boxes - then we can confine height of iterations' legend and scroll down won't happen.

And, can you give different colors to next boundary temperature and residual luminosity?

@jaladh-singhal
Copy link
Member

Also I noticed plot labels were not present and dots were present in luminosity plots (that we decided to remove) - I hope you'll fix that since you showed us plot with these changes right?

@jaladh-singhal
Copy link
Member

And one last thing - the 1st cell which widens entire notebook shouldn't be done. By default 1200px width of jupyter notebook should be respected because as per design principles, our eyes can only see that much width in one glance. This is the very reason many websites you see out there have content horizontally centered and confined in fixed width on desktop screens (leaving empty space on both left & right ends).

And you also need not to constraint height of vbox to 1000px - plotly plots are smart enough to adapt according to space available to them so let them be.

@atharva-2001
Copy link
Member Author

atharva-2001 commented Jun 18, 2021 •
edited
Loading

Thanks for reviewing it!
I'm afraid plotly only allows one legend per graph at the moment. (Link to the discussion). I did however find a workaround, pinning the right margin of the plasma plot to 135, almost aligns the plots perfectly, but if someone changes the width of the cell, the plots move from the perfect alignment just a little bit.
I'm sorry for changing the width of the cell to 100%, I had done that to take a screenshot(to show the entire plot in one picture) :)

I found a way to show the plots on nbviewer as well in the documentation, but I did it by suppressing errors that showed up.
Here is the link to the documentation I built to show the plots.
Here is the link to the notebook on nbviewer.
If you are not able to view the plots, please try this link, as the notebook was probably not updated.

@atharva-2001 atharva-2001 force-pushed the convergence_plots_demo branch from 05f3617 to 32f94ce Compare June 18, 2021 15:17
@atharva-2001 atharva-2001 force-pushed the convergence_plots_demo branch from 32f94ce to 49637a7 Compare June 18, 2021 15:21
@atharva-2001 atharva-2001 mentioned this pull request Aug 19, 2021
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaladh-singhal jaladh-singhal jaladh-singhal left review comments

@jamesgillanders jamesgillanders Awaiting requested review from jamesgillanders

@MarkMageeAstro MarkMageeAstro Awaiting requested review from MarkMageeAstro

@marxwillia marxwillia Awaiting requested review from marxwillia

Assignees
No one assigned
Labels
feature visualization work in progress 🚧
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.