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

Add modified Kromer plot to work with widgets #1241

Merged
merged 24 commits into from
Dec 21, 2020

Conversation

jaladh-singhal
Copy link
Member

@jaladh-singhal jaladh-singhal commented Jul 17, 2020

This PR adds Kromer plot from tardisanalysis in simplified structure so that it can work with widgets being developed. The main changes made are:

  • It doesn't depend on minimal_model, can be created directly from a Simulation or HDF
  • Same object contains data of both real & virtual mode
  • Produces interactive plots (using plotly) besides static matplotlib plots
  • Incorporates the structure of KromerPlotter which is highly simplified and more readable by using @marxwillia's work at New Kromer Plot Implementation tardisanalysis#35

To-dos

  • Construct object independent of minimal_model
  • Make same object store data of both real & virtual mode
  • Create classmethod that can create from a Simulation object
  • Create classmethod that can create from a HDF
  • Add proper & relevant documentation/docstrings to lots of properties
  • Optimize the matplotlib implementation of plot
  • Create interactive version of matplotlib in plotly

Note

Since this PR is already pretty bulky, documentation notebook and unit tests will be added in separate PR

@codecov
Copy link

codecov bot commented Jul 24, 2020

Codecov Report

Merging #1241 (7bc3afb) into master (575c228) will decrease coverage by 2.33%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1241      +/-   ##
==========================================
- Coverage   72.38%   70.04%   -2.34%     
==========================================
  Files          66       67       +1     
  Lines        5098     5312     +214     
==========================================
+ Hits         3690     3721      +31     
- Misses       1408     1591     +183     
Impacted Files Coverage Δ
tardis/tardis/model/base.py 88.29% <0.00%> (ø)
tardis/tardis/montecarlo/base.py 84.78% <0.00%> (ø)
tardis/tardis/montecarlo/spectrum.py 70.00% <0.00%> (ø)
tardis/tardis/widgets/kromer_plot.py 14.48% <0.00%> (ø)

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 575c228...7bc3afb. Read the comment docs.

@jaladh-singhal jaladh-singhal force-pushed the widgets/kromer_plot branch 2 times, most recently from 238bca6 to f1d18c6 Compare November 10, 2020 19:12
@jaladh-singhal jaladh-singhal marked this pull request as ready for review November 13, 2020 08:05
should be a quantity having units of Angstrom, containing two
values - lower lambda and upper lambda i.e.
[lower_lambda, upper_lambda] * u.AA
distance : astropy.Quantity
Copy link
Contributor

Choose a reason for hiding this comment

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

@marxwillia test that the distance actually works.

self.plot_frequency = self.data[packets_mode].spectrum_frequency

# Filter their plottable range based on packet_wvl_range specified
if packet_wvl_range:
Copy link
Contributor

Choose a reason for hiding this comment

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

Plan is to merge, make a separate issue, fix any colormap bugs in separate PR.

@marxwillia marxwillia merged commit bf8f7ad into tardis-sn:master Dec 21, 2020
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
…plot

Add modified Kromer plot to work with widgets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants