Add option to add Observed Spectrum #1761

atharva-2001 · 2021-07-29T13:43:39Z

This PR adds an option to plot an observed spectrum in the SDEC plot.

Description

The option asks for a tuple of Astropy quantities, containing the wavelength and the luminosity for the observed spectrum and adds a line plot displaying the Observed Spectrum.

Motivation and context

How has this been tested?

Testing pipeline.
Other.

Examples

documentation: https://atharva-2001.github.io/tardis/branch/observed_spectrum/io/visualization/sdec_plot.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.

codecov · 2021-07-29T13:53:41Z

Codecov Report

Merging #1761 (907785d) into master (e6a8af2) will decrease coverage by 0.13%.
The diff coverage is n/a.

❗ Current head 907785d differs from pull request most recent head c737c2d. Consider uploading reports for the commit c737c2d to get more accurate results

@@            Coverage Diff             @@
##           master    #1761      +/-   ##
==========================================
- Coverage   58.38%   58.25%   -0.14%     
==========================================
  Files          66       66              
  Lines        6705     6721      +16     
==========================================
  Hits         3915     3915              
- Misses       2790     2806      +16

Impacted Files	Coverage Δ
tardis/tardis/visualization/tools/sdec_plot.py	`9.52% <0.00%> (-0.43%)`	⬇️

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 e6a8af2...c737c2d. Read the comment docs.

tardis-bot · 2021-08-02T15:59:47Z

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.

jaladh-singhal

This is a good start @atharva-2001 but I think there are few things unclear at your end - feel free to ask.

Also can you document this in SDEC notebook so that we can try this functionalities out?

tardis/visualization/tools/sdec_plot.py

jaladh-singhal · 2021-08-05T19:34:45Z

Black check pipeline fails

review-notebook-app · 2021-08-16T14:57:28Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

jamesgillanders

We also need to think about the colour scheme, as cyan does not look great for the observed spectrum

tardis/visualization/tools/sdec_plot.py

review-notebook-app · 2021-09-17T11:21:23Z

View / edit / reply to this conversation on ReviewNB

jaladh-singhal commented on 2021-09-17T11:21:23Z
----------------------------------------------------------------

Instead of this long cell, put these numbers in a csv/ascii file named "demo_observed_spectrum.csv" (at same level as demo.hdf) and read from there using pandas and then create quantitites

jaladh-singhal

Good job @atharva-2001, colors look nice to me - it's closer to be merged.

You need to resolve merge conflict and address my comment about putting data in a file instead of cell.

docs/io/visualization/sdec_plot.ipynb

jaladh-singhal · 2021-09-27T18:23:46Z

docs/io/visualization/sdec_plot.ipynb

@@ -13,8 +13,8 @@
   "execution_count": null,


Why not directly assign vertical slices of data to both observed_spectrum_wavelength and observed_spectrum_flux. Please look into ways of indexing ndarray to learn how to do it. If that's not possible with numpy, there's always pandas to do read almost all file formats. then you can just assign series to both variables.

Always remember, iterating is the last we should do with numpy & pandas as it reduces speed.

Reply via ReviewNB

jaladh-singhal

@atharva-2001 thank you for reading it from file - it's much cleaner now.

I've some concerns about your implementation of reading - I guess it should be quick to address. You can always take help related to this on computing channel if you want.

PS: You forgot to hit re-request review button this time too but I did review it 😅 - make sure to remember this time.

sourcery-ai · 2021-09-28T13:29:37Z

Sourcery Code Quality Report

❌ Merging this PR will decrease code quality in the affected files by 1.32%.

Quality metrics	Before	After	Change
Complexity	8.79 🙂	9.23 🙂	0.44 👎
Method Length	159.52 😞	163.74 😞	4.22 👎
Working memory	14.88 😞	15.08 😞	0.20 👎
Quality	45.69% 😞	44.37% 😞	-1.32% 👎

Other metrics	Before	After	Change
Lines	1737	1797	60

Changed files	Quality Before	Quality After	Quality Change
tardis/visualization/tools/sdec_plot.py	45.69% 😞	44.37% 😞	-1.32% 👎

Here are some functions in these files that still need a tune-up:

File	Function	Complexity	Length	Working Memory	Quality	Recommendation
tardis/visualization/tools/sdec_plot.py	SDECPlotter._calculate_plotting_data	14 🙂	634 ⛔	21 ⛔	23.78% ⛔	Try splitting into smaller methods. Extract out complex expressions
tardis/visualization/tools/sdec_plot.py	SDECPlotter._parse_species_list	26 😞	186 😞	17 ⛔	27.78% 😞	Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
tardis/visualization/tools/sdec_plot.py	SDECPlotter.generate_plot_ply	10 🙂	268 ⛔	17 ⛔	34.73% 😞	Try splitting into smaller methods. Extract out complex expressions
tardis/visualization/tools/sdec_plot.py	SDECData.from_hdf	3 ⭐	291 ⛔	32 ⛔	34.76% 😞	Try splitting into smaller methods. Extract out complex expressions
tardis/visualization/tools/sdec_plot.py	SDECPlotter.generate_plot_mpl	10 🙂	223 ⛔	16 ⛔	38.11% 😞	Try splitting into smaller methods. Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

⭐ excellent
🙂 good
😞 poor
⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.

Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

atharva-2001 · 2021-09-28T13:48:14Z

@jaladh-singhal Thank you for reviewing it!
I am sorry for not requesting reviews, I was about to do it once the docs were built but then forgot about it. There seems to be a small problem with my documentation build, I would request reviews once that's fixed!

jaladh-singhal · 2021-10-01T13:23:40Z

I am sorry for not requesting reviews, I was about to do it once the docs were built but then forgot about it. There seems to be a small problem with my documentation build, I would request reviews once that's fixed!

@atharva-2001 You don't really need to wait for docs to build. First, because we don't reviewe immediately anyways 😄 and second because we (atleast I) review rendered notebook using nbreview - docs build is just sanity check in this case.

jaladh-singhal

Looks neater now! Good work!

jamesgillanders

Looks good to me - if you fix the few minor comments then I'd be happy to merge!

tardis/visualization/tools/sdec_plot.py

jamesgillanders

looks good

This reverts commit 45bb948.

atharva-2001 added visualization work in progress 🚧 labels Jul 29, 2021

atharva-2001 force-pushed the observed_spectrum branch from aca7d56 to 1a53a16 Compare August 2, 2021 12:20

andrewfullard requested review from jaladh-singhal and jamesgillanders August 2, 2021 15:59

atharva-2001 force-pushed the observed_spectrum branch from 1a53a16 to 4b5caea Compare August 5, 2021 16:02

jaladh-singhal requested changes Aug 5, 2021

View reviewed changes

jaladh-singhal requested a review from MarkMageeAstro August 5, 2021 19:34

atharva-2001 force-pushed the observed_spectrum branch 4 times, most recently from 3ee40ac to bae1e3c Compare August 13, 2021 11:58

atharva-2001 force-pushed the observed_spectrum branch 2 times, most recently from e726ec0 to b039368 Compare August 16, 2021 15:06

jamesgillanders requested changes Aug 19, 2021

View reviewed changes

atharva-2001 force-pushed the observed_spectrum branch from b039368 to 01191ac Compare August 20, 2021 13:40

atharva-2001 force-pushed the observed_spectrum branch from c43f5fa to 19dfbe5 Compare September 2, 2021 09:16

atharva-2001 force-pushed the observed_spectrum branch from e08bebf to d22e044 Compare September 16, 2021 06:51

sourcery-ai bot mentioned this pull request Sep 16, 2021

Add option to add Observed Spectrum (Sourcery refactored) #1796

Closed

atharva-2001 force-pushed the observed_spectrum branch from d22e044 to 92afff7 Compare September 16, 2021 06:54

jaladh-singhal requested changes Sep 17, 2021

View reviewed changes

atharva-2001 force-pushed the observed_spectrum branch from 946bb12 to 973d18f Compare September 18, 2021 07:15

jaladh-singhal reviewed Sep 27, 2021

View reviewed changes

jaladh-singhal requested changes Sep 27, 2021

View reviewed changes

jaladh-singhal requested a review from jamesgillanders September 27, 2021 18:28

atharva-2001 force-pushed the observed_spectrum branch 2 times, most recently from fc6d526 to 8a0b575 Compare September 28, 2021 13:28

atharva-2001 force-pushed the observed_spectrum branch 5 times, most recently from 84f01a0 to cb30a3a Compare October 1, 2021 12:07

jaladh-singhal approved these changes Oct 1, 2021

View reviewed changes

jamesgillanders requested changes Oct 1, 2021

View reviewed changes

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

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

atharva-2001 force-pushed the observed_spectrum branch 2 times, most recently from 907785d to 835477b Compare October 2, 2021 03:22

atharva-2001 requested a review from jamesgillanders October 2, 2021 03:26

atharva-2001 force-pushed the observed_spectrum branch from 835477b to bc5559f Compare October 2, 2021 03:36

atharva-2001 added 7 commits October 4, 2021 12:48

Add option to plot observed spectrum

8d45572

Fix docstrings, change spectrum units and do not check type

345e773

Demonstrating observed_spectrum in the documentation

ba743d4

Specify units in docstrings

ac47b8e

Change color scale

1564dc0

Read data for plotting observed spectrum from file

6f9eadb

Fix typo

c737c2d

atharva-2001 force-pushed the observed_spectrum branch from bc5559f to c737c2d Compare October 4, 2021 07:19

jamesgillanders approved these changes Oct 4, 2021

View reviewed changes

jamesgillanders merged commit 45bb948 into tardis-sn:master Oct 4, 2021

jamesgillanders pushed a commit that referenced this pull request Oct 4, 2021

Revert "Add option to add Observed Spectrum (#1761)"

79760c6

This reverts commit 45bb948.

jamesgillanders mentioned this pull request Oct 4, 2021

Revert "Add option to add Observed Spectrum" #1805

Merged

jamesgillanders pushed a commit that referenced this pull request Oct 4, 2021

Revert "Add option to add Observed Spectrum (#1761)" (#1805)

44acad2

This reverts commit 45bb948.

atharva-2001 mentioned this pull request Oct 8, 2021

Add option to add Observed Spectrum (2) #1810

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to add Observed Spectrum #1761

Add option to add Observed Spectrum #1761

atharva-2001 commented Jul 29, 2021 •

edited

Loading

codecov bot commented Jul 29, 2021 •

edited

Loading

tardis-bot commented Aug 2, 2021

jaladh-singhal left a comment

jaladh-singhal commented Aug 5, 2021

review-notebook-app bot commented Aug 16, 2021

jamesgillanders left a comment

review-notebook-app bot commented Sep 17, 2021

jaladh-singhal left a comment

jaladh-singhal Sep 27, 2021

jaladh-singhal left a comment •

edited

Loading

sourcery-ai bot commented Sep 28, 2021

atharva-2001 commented Sep 28, 2021 •

edited

Loading

jaladh-singhal commented Oct 1, 2021 •

edited

Loading

jaladh-singhal left a comment

jamesgillanders left a comment

jamesgillanders left a comment

Add option to add Observed Spectrum #1761

Add option to add Observed Spectrum #1761

Conversation

atharva-2001 commented Jul 29, 2021 • edited Loading

codecov bot commented Jul 29, 2021 • edited Loading

Codecov Report

tardis-bot commented Aug 2, 2021

jaladh-singhal left a comment

Choose a reason for hiding this comment

jaladh-singhal commented Aug 5, 2021

review-notebook-app bot commented Aug 16, 2021

jamesgillanders left a comment

Choose a reason for hiding this comment

review-notebook-app bot commented Sep 17, 2021

jaladh-singhal left a comment

Choose a reason for hiding this comment

jaladh-singhal Sep 27, 2021

Choose a reason for hiding this comment

jaladh-singhal left a comment • edited Loading

Choose a reason for hiding this comment

sourcery-ai bot commented Sep 28, 2021

Sourcery Code Quality Report

Legend and Explanation

atharva-2001 commented Sep 28, 2021 • edited Loading

jaladh-singhal commented Oct 1, 2021 • edited Loading

jaladh-singhal left a comment

Choose a reason for hiding this comment

jamesgillanders left a comment

Choose a reason for hiding this comment

jamesgillanders left a comment

Choose a reason for hiding this comment

atharva-2001 commented Jul 29, 2021 •

edited

Loading

codecov bot commented Jul 29, 2021 •

edited

Loading

jaladh-singhal left a comment •

edited

Loading

atharva-2001 commented Sep 28, 2021 •

edited

Loading

jaladh-singhal commented Oct 1, 2021 •

edited

Loading