Demonstrating the `nelements` and the `species_list` functionality in the SDEC plot notebook #1665

atharva-2001 · 2021-06-24T11:14:39Z

This PR adds documentation for nelements and species_list options in the SDEC plot notebook.

Description

The nelements functionality was added in PR #1506
The species_list functionality was added in PR #1539
This PR demonstrates these two functionalities in the notebook.

Motivation and context

How has this been tested?

Testing pipeline.
Other.

Examples

Here is the link to the notebook in my documentation
Link to the notebook on nbviewer

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.

…lots notebook and fixing the link to the configuration schema

review-notebook-app · 2021-06-24T11:14:44Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

tardis-bot · 2021-06-24T11:18:53Z

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.

tardis-bot · 2021-06-24T11:18:53Z

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.

tardis-bot · 2021-06-24T11:18:53Z

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 · 2021-06-24T11:26:22Z

Codecov Report

Merging #1665 (553ef19) into master (05f759a) will decrease coverage by 0.63%.
The diff coverage is n/a.

❗ Current head 553ef19 differs from pull request most recent head 29706dc. Consider uploading reports for the commit 29706dc to get more accurate results

@@            Coverage Diff             @@
##           master    #1665      +/-   ##
==========================================
- Coverage   61.89%   61.26%   -0.64%     
==========================================
  Files          62       63       +1     
  Lines        5732     5798      +66     
==========================================
+ Hits         3548     3552       +4     
- Misses       2184     2246      +62

Impacted Files	Coverage Δ
tardis/tardis/montecarlo/spectrum.py	`65.51% <0.00%> (-3.72%)`	⬇️
tardis/tardis/simulation/base.py	`83.33% <0.00%> (-1.84%)`	⬇️
tardis/tardis/model/base.py	`88.29% <0.00%> (ø)`
tardis/tardis/model/density.py	`92.53% <0.00%> (ø)`
tardis/tardis/montecarlo/montecarlo_numba/base.py	`28.73% <0.00%> (ø)`
...rdis/tardis/montecarlo/montecarlo_numba/vpacket.py	`18.36% <0.00%> (ø)`
...dis/tardis/montecarlo/montecarlo_numba/r_packet.py	`25.43% <0.00%> (ø)`
.../tardis/montecarlo/montecarlo_numba/interaction.py	`29.72% <0.00%> (ø)`
...dis/montecarlo/montecarlo_numba/formal_integral.py	`49.82% <0.00%> (ø)`
tardis/tardis/scripts/tardis.py	`0.00% <0.00%> (ø)`
... and 1 more

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 05f759a...29706dc. Read the comment docs.

jaladh-singhal

Thanks for making this PR - it was long due!

Few suggestions:

Add these two sections also on matplotlib section (besides plotly) because that is shown first and is preferred in research papers
Why have you used real packets_mode? SDEC plot looks better with virtual packets mode as you might have noticed (due to noise reduction) and it's also default, please change that. I guess it's possibly because you're using tardis_example.yml that have virt packet logging disabled but SDEC plot and line info widget expects opposite - create an issue for that and work on it if you like :)

jaladh-singhal · 2021-06-25T16:15:59Z

Also please check what happens when both nelments and species_list are specified - AFAIR one takes precedence over other and it should be documented as well

MarkMageeAstro

Good job, @atharva-2001. My only comment is that I think for the nelements/ion plots we should set the wavelength range. It'll make it easier to see the different colours.

jaladh-singhal

Thanks for the changes. One minor but thing: it'll be better if you put 3 cells you added in matpltolib section, before "plotting flux instead of luminosity" since it and hiding spectrum are not as important as nelments and species_list is

Also I see you're using virtual packets now - that's good but don't forget creating issue for this:

I guess it's possibly because you're using tardis_example.yml that have virt packet logging disabled but SDEC plot and line info widget expects opposite - create an issue for that and work on it if you like :)

MarkMageeAstro

I can't leave comments on the notebook, so I'm just putting them all here. A few small grammar changes.

There are a few instances throughout where the plot is just referred to as 'SDEC plot', but there should be either 'an' or 'the' in front of it. These are two examples:

"Now, import the plotting interface for SDEC plot..." should be "Now, import the plotting interface for the SDEC plot..."
"By default, SDEC plot is produced..." should be "By default, an SDEC plot is produced..."

"Since packets_mode is 1st argument..." -> "Since packets_mode is the first argument..."
"By default, modeled spectrum is shown in SDEC plot." -> "By default, the model spectrum is shown in SDEC plot."
"This method takes exact same arguments" -> "This method takes the exact same arguments"
"In similar manner" -> "In a similar manner"
" thereby providing you more" -> " thereby providing you with more"

jaladh-singhal

Thanks for all the changes. Looks good to me!

Demonstrating the `nelements` and the `species_list` functionality in the SDEC plot notebook

demonstrating the nelements and the species_list option in the SDEC p…

52fbbfa

…lots notebook and fixing the link to the configuration schema

atharva-2001 changed the title ~~demonstrating the nelements and the species_list option in the SDEC p…~~ Demonstrating the nelements and the species_list functionality in the SDEC plot notebook Jun 24, 2021

atharva-2001 requested review from jaladh-singhal, jamesgillanders, MarkMageeAstro and marxwillia June 24, 2021 11:18

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

jaladh-singhal requested changes Jun 25, 2021

View reviewed changes

showing plots in matplotlib

67c4824

atharva-2001 force-pushed the sdec_docs branch from 82e48e1 to 67c4824 Compare June 28, 2021 06:46

andrewfullard added documentation visualization labels Jun 28, 2021

MarkMageeAstro requested changes Jun 28, 2021

View reviewed changes

jaladh-singhal force-pushed the master branch from 2907807 to d5ac38a Compare July 1, 2021 18:47

jaladh-singhal reviewed Jul 1, 2021

View reviewed changes

atharva-2001 force-pushed the sdec_docs branch from 16aeff7 to 022d57b Compare July 1, 2021 19:29

change wavelength range, rearrange cells

e2a4b9c

atharva-2001 force-pushed the sdec_docs branch from 022d57b to e2a4b9c Compare July 1, 2021 19:38

atharva-2001 requested review from jaladh-singhal and MarkMageeAstro July 1, 2021 20:00

atharva-2001 mentioned this pull request Jul 7, 2021

Enabling virtual_packet_logging by default in the visualization folder #1693

Merged

10 tasks

MarkMageeAstro requested changes Jul 7, 2021

View reviewed changes

wkerzendorf force-pushed the master branch from a7d0a9b to c0dbc5b Compare July 7, 2021 15:16

atharva-2001 closed this Jul 8, 2021

atharva-2001 force-pushed the sdec_docs branch from e2a4b9c to 05f759a Compare July 8, 2021 06:03

atharva-2001 reopened this Jul 8, 2021

fix grammatical errors

29706dc

jaladh-singhal approved these changes Jul 8, 2021

View reviewed changes

jaladh-singhal requested a review from MarkMageeAstro July 8, 2021 19:50

MarkMageeAstro approved these changes Jul 16, 2021

View reviewed changes

marxwillia merged commit d3f13ed into tardis-sn:master Jul 16, 2021

atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021

Merge pull request tardis-sn#1665 from atharva-2001/sdec_docs

515c03e

Demonstrating the `nelements` and the `species_list` functionality in the SDEC plot notebook

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Demonstrating the `nelements` and the `species_list` functionality in the SDEC plot notebook #1665

Demonstrating the `nelements` and the `species_list` functionality in the SDEC plot notebook #1665

atharva-2001 commented Jun 24, 2021 •

edited

Loading

review-notebook-app bot commented Jun 24, 2021

tardis-bot commented Jun 24, 2021

tardis-bot commented Jun 24, 2021

tardis-bot commented Jun 24, 2021

codecov bot commented Jun 24, 2021 •

edited

Loading

jaladh-singhal left a comment

jaladh-singhal commented Jun 25, 2021

MarkMageeAstro left a comment

jaladh-singhal left a comment •

edited

Loading

MarkMageeAstro left a comment

jaladh-singhal left a comment

Demonstrating the nelements and the species_list functionality in the SDEC plot notebook #1665

Demonstrating the nelements and the species_list functionality in the SDEC plot notebook #1665

Conversation

atharva-2001 commented Jun 24, 2021 • edited Loading

review-notebook-app bot commented Jun 24, 2021

tardis-bot commented Jun 24, 2021

tardis-bot commented Jun 24, 2021

tardis-bot commented Jun 24, 2021

codecov bot commented Jun 24, 2021 • edited Loading

Codecov Report

jaladh-singhal left a comment

Choose a reason for hiding this comment

jaladh-singhal commented Jun 25, 2021

MarkMageeAstro left a comment

Choose a reason for hiding this comment

jaladh-singhal left a comment • edited Loading

Choose a reason for hiding this comment

MarkMageeAstro left a comment

Choose a reason for hiding this comment

jaladh-singhal left a comment

Choose a reason for hiding this comment

Demonstrating the `nelements` and the `species_list` functionality in the SDEC plot notebook #1665

Demonstrating the `nelements` and the `species_list` functionality in the SDEC plot notebook #1665

atharva-2001 commented Jun 24, 2021 •

edited

Loading

codecov bot commented Jun 24, 2021 •

edited

Loading

jaladh-singhal left a comment •

edited

Loading