Integrate custom abundance widget as a full module #1751

yuyizheng1112 · 2021-07-22T15:18:07Z

This PR integrates the codes of custom abundance widget from notebook to .py module. The notebook is in pr #1626.

Description

Add custom_abundance.py module to create widget object.
Add Custom Abundance Widget to using_widgets.rst to demonstrate how to use the widget. See the documentation page here.
Add abundance_widget.ipynb notebook to demonstrate how to generate the widget. See the documentation page here.

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-22T15:28:15Z

Codecov Report

Merging #1751 (8138381) into master (1c3322b) will decrease coverage by 4.24%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1751      +/-   ##
==========================================
- Coverage   62.65%   58.40%   -4.25%     
==========================================
  Files          65       66       +1     
  Lines        6036     6631     +595     
==========================================
+ Hits         3782     3873      +91     
- Misses       2254     2758     +504

Impacted Files	Coverage Δ
tardis/tardis/visualization/widgets/util.py	`72.30% <0.00%> (-13.06%)`	⬇️
tardis/tardis/simulation/base.py	`83.17% <0.00%> (ø)`
...dis/tardis/visualization/tools/convergence_plot.py	`88.67% <0.00%> (ø)`
...s/tardis/visualization/widgets/custom_abundance.py	`13.83% <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 ea86abf...8138381. Read the comment docs.

tardis/visualization/widgets/custom_abundance.py

tardis-bot · 2021-07-23T14:21:48Z

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.

review-notebook-app · 2021-08-03T14:52:23Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

MarkMageeAstro

Very nice work. I haven't looked at the code itself in detail because I have a few comments regarding bugs/design.

Need units for the density in the plot. It should be g/cm3
The description of the 'apply to other shells' function should be more descriptive. It looks like it only applies whichever ones are locked, is that true?
The edit density functionality didn't work for me. The plot never updated to show the new density I had put in. This is true for editing the density of a single shell and using different functions to calculate the densities in multiple shells
If I have more than 10 elements, the colours in the plot start to repeat, which makes it difficult to tell them apart. Can we have a different colour scheme? Maybe a gradient? Although even with a gradient, it'll be difficult to tell apart if there are many elements. Basically, I don't have a good solution to this.
When calculating and plotting the densities, there must be some t0 assumed. This should also be the default value in the box where it asks you to put in a t0 and stated somewhere. Perhaps the y label should be 'Density at XX' where XX = t0.
I think it makes sense to keep the points in the plot because then you can easily see where the cells change. If there are no points and I have a constant fractional abundance, then I can't see where each cell begins and ends
There is still an issue with adding cells below the boundary. This is raised in issue Density calculated below photosphere for Branch W7 density #1734. Not sure what to do about it in the mean time.
I think the output functionality should add the file extension. e.g. if I save file path = 'test' then it should save as 'test.csvy'
If I overwrite existing cells and therefore change the number of cells in the model, the shell number box doesn't update to reflect that. For example, say I have a model with 20 cells as in the example and I highlight cell 2 in the plot. Then I update the model to overwrite all cells into one big cell. In this case I get an index error and it crashes

This may seem like a lot, but overall it's very impressive, good job!

jaladh-singhal · 2021-08-05T19:42:21Z

@yuyizheng1112 black check workflow is failing. Also have you added changes we discussed to widget layout - I'm planning to review once that is done?

yuyizheng1112 · 2021-08-06T10:14:23Z

@MarkMageeAstro
2. That's true. Since it is a bit confusing, I am working on a new layout.
5. The calculation of density profile doesn't seem to require t_0, so I put it in output part.
6. Sure. I'll retain the markers.
And I'm working on the rest staff.

wkerzendorf

I would also suggest that we use the json schemas to see what types of density profiles exist.

tardis/visualization/widgets/custom_abundance.py

wkerzendorf · 2021-08-12T13:23:43Z

tardis/visualization/widgets/custom_abundance.py

+YAML_DELIMITER = "---"
+
+
+class CustomYAML(yaml.YAMLObject):


why do we need a new YAML file?

This class generates a YAML object for writing YAML portion when saving the model file.

tardis/visualization/widgets/custom_abundance.py

wkerzendorf · 2021-08-12T13:30:43Z

tardis/visualization/widgets/custom_abundance.py

+            CustomAbundanceWidget
+        """
+        csvy_model_config, csvy_model_data = load_csvy(fpath)
+        base_dir = os.path.abspath(os.path.dirname(__file__))


lets use tardis.__path__ here as BASE_DIR at the beginning of the file.

and use pathlib

@wkerzendorf I think I can use pathlib when writing CSVY output file, but why should we use pathlib in from_csvy method?

wkerzendorf · 2021-08-12T13:39:11Z

tardis/visualization/widgets/custom_abundance.py

+
+            # Add YAML delimiter
+            splits = yaml_output.split("\n")
+            splits[0] = splits[-1] = YAML_DELIMITER


not clear what that line does (it might be right - just don't understand it)

It replaces the first and the last line with --- in the CSVY file.
The original first line is a YAML tag: !!python/object:tardis.visualization.widgets.custom_abundance.CustomYAML and the original last line is blank, so I add this line to replace them and add yaml delimiter.

@yuyizheng1112 probably it'll be better to rename splits as yaml_output_snippets

yuyizheng1112 · 2021-08-12T15:54:45Z

I would also suggest that we use the json schemas to see what types of density profiles exist.

@wkerzendorf Can I do this in another new PR?

jaladh-singhal

This is a very excellent work @yuyizheng1112 - congratulations on writing such a huge module single-handedly. 🎉

Your code organisation is nice, you have righlty splitted functionality in appropriate methods, and I really liked the pattern you followed in naming so many widget components. The code documentation is also fairly good except for few changes that I commented. The docs also look great with the examples.

My comment count is really high 😅 but they are mostly small nitpicky requests that shouldn't take much time to address. Feel free to let me know if anything is unclear

tardis/visualization/widgets/custom_abundance.py

docs/io/visualization/using_widgets.rst

jaladh-singhal · 2021-08-12T16:18:57Z

docs/io/visualization/index.rst

Suggested change

Generating Widgets in a Notebook <generating_widgets>

abundance_widget

Generating Custom Abundance Widget <abundance_widget>

Generating Data Exploration Widgets <generating_widgets>

For now let's keep it this way.

Also can you edit the title of generating_widgets.ipynb to same?

I edit the title of both generating_widgets.ipynb and abundance_widget.ipynb. I also clear the output and commented the cell that generates shell_info_widget from HDF file. Please tell me if you don't want to do that.

Since you edited the title there was no need of mentioning explicit title in TOC but NVM

Co-authored-by: Jaladh Singhal <[email protected]>

jaladh-singhal

Thank you for addressing all of my comments - docstrings look much better now.

jamesgillanders

This is an extremely well-structured piece of code @yuyizheng1112 - fantastic work! I have no issues with merging this

tardis/visualization/widgets/custom_abundance.py

marxwillia requested changes Jul 22, 2021

View reviewed changes

tardis/visualization/widgets/custom_abundance.py Outdated Show resolved Hide resolved

tardis/visualization/widgets/custom_abundance.py Outdated Show resolved Hide resolved

tardis/visualization/widgets/custom_abundance.py Show resolved Hide resolved

yuyizheng1112 added the visualization label Jul 23, 2021

yuyizheng1112 requested a review from jaladh-singhal July 23, 2021 14:21

yuyizheng1112 requested review from jamesgillanders and MarkMageeAstro July 30, 2021 14:18

yuyizheng1112 force-pushed the abundance-widget-module branch from 8435789 to d68db9d Compare August 3, 2021 14:44

MarkMageeAstro requested changes Aug 5, 2021

View reviewed changes

yuyizheng1112 added 15 commits August 11, 2021 20:31

Integrate custom abundance widget as a full module

4355d7b

Add docstrings

f56e215

Fix minor bugs

f7e196d

Change the plot to step graph

387b3ce

Fix adding element bug

eb9c576

Add documentation

1fc4e5d

Add radio buttons to switch apply type

391e79a

Adjust the layout

a2d7970

Fix density plot bug

121a62f

Fix shell overwriting bug

838dd92

Fix density interpolation

20f2892

Add colorscale, add multi-shell selection in plot, load t_0

55841ce

Add docstrings to new methods

a54308b

Add documentation

c6adf31

Black format

8cc5f88

yuyizheng1112 force-pushed the abundance-widget-module branch from aaaca06 to 8cc5f88 Compare August 11, 2021 12:32

yuyizheng1112 added 2 commits August 12, 2021 14:48

Add new documentation

08d77fe

Add images for documentation

7bc3fd3

yuyizheng1112 requested review from marxwillia and MarkMageeAstro August 12, 2021 06:52

[build docs]

de7414f

yuyizheng1112 marked this pull request as ready for review August 12, 2021 07:09

yuyizheng1112 added 2 commits August 12, 2021 15:32

[build docs]

c9a5244

Clear widget output and add GIF

a340577

wkerzendorf reviewed Aug 12, 2021

View reviewed changes

jaladh-singhal requested changes Aug 12, 2021

View reviewed changes

yuyizheng1112 and others added 6 commits August 13, 2021 11:08

Use pathlib and made minor changes

0591907

Update tardis/visualization/widgets/custom_abundance.py

fedac9c

Co-authored-by: Jaladh Singhal <[email protected]>

Move Timer class and chang docstrings

d8eb86d

Rename some functions and variables

7a28d08

docstring changes to util.py

05f3474

Edit documentation and notebook title

264d3f7

yuyizheng1112 requested review from wkerzendorf and jaladh-singhal August 13, 2021 07:59

yuyizheng1112 added 4 commits August 13, 2021 16:00

Perform black format

3eecf9f

[build docs]

fa0b2ef

Correct the section name

a6829fe

[build docs]

3d4d029

jaladh-singhal approved these changes Aug 19, 2021

View reviewed changes

jamesgillanders approved these changes Aug 19, 2021

View reviewed changes

tardis/visualization/widgets/custom_abundance.py Outdated Show resolved Hide resolved

Change docstring of time_0

8138381

jamesgillanders merged commit eb73057 into tardis-sn:master Aug 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate custom abundance widget as a full module #1751

Integrate custom abundance widget as a full module #1751

yuyizheng1112 commented Jul 22, 2021 •

edited

Loading

codecov bot commented Jul 22, 2021 •

edited

Loading

tardis-bot commented Jul 23, 2021

review-notebook-app bot commented Aug 3, 2021

MarkMageeAstro left a comment

jaladh-singhal commented Aug 5, 2021

yuyizheng1112 commented Aug 6, 2021

wkerzendorf left a comment

wkerzendorf Aug 12, 2021

yuyizheng1112 Aug 12, 2021

wkerzendorf Aug 12, 2021

wkerzendorf Aug 12, 2021

yuyizheng1112 Aug 13, 2021

wkerzendorf Aug 12, 2021

yuyizheng1112 Aug 12, 2021

jaladh-singhal Aug 12, 2021

yuyizheng1112 commented Aug 12, 2021

jaladh-singhal left a comment •

edited

Loading

jaladh-singhal Aug 12, 2021

jaladh-singhal Aug 12, 2021

yuyizheng1112 Aug 13, 2021

jaladh-singhal Aug 19, 2021

jaladh-singhal left a comment

jamesgillanders left a comment

-    Generating Widgets in a Notebook <generating_widgets>
-    abundance_widget
+    Generating Custom Abundance Widget <abundance_widget>
+    Generating Data Exploration Widgets <generating_widgets>

Integrate custom abundance widget as a full module #1751

Integrate custom abundance widget as a full module #1751

Conversation

yuyizheng1112 commented Jul 22, 2021 • edited Loading

codecov bot commented Jul 22, 2021 • edited Loading

Codecov Report

tardis-bot commented Jul 23, 2021

review-notebook-app bot commented Aug 3, 2021

MarkMageeAstro left a comment

Choose a reason for hiding this comment

jaladh-singhal commented Aug 5, 2021

yuyizheng1112 commented Aug 6, 2021

wkerzendorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyizheng1112 commented Aug 12, 2021

jaladh-singhal left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaladh-singhal left a comment

Choose a reason for hiding this comment

jamesgillanders left a comment

Choose a reason for hiding this comment

yuyizheng1112 commented Jul 22, 2021 •

edited

Loading

codecov bot commented Jul 22, 2021 •

edited

Loading

jaladh-singhal left a comment •

edited

Loading