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 line info widget to widgets subpackage #1264

Merged
merged 25 commits into from
Sep 21, 2020

Conversation

jaladh-singhal
Copy link
Member

@jaladh-singhal jaladh-singhal commented Aug 5, 2020

This PR aims to put line info widget which was being prepared in a notebook (#1187) as a module within the widgets subpackage so that it can be generated easily by constructing the widget object.

The main changes made are:

  • Edited analysis.py to make it work with line inf widget:

    • Classmethod from_model did not take packet_filter_mode as argument but it is required for initializing LastLineInteraction (as __init__() takes it), so added it
    • Added filter mode packet_in_nu and renamed packet_nu to packet_out_nu since it only concerned with output_nu
    • output_nu and last_interaction_in_nu didn't always had "Hz" unit, so made sure that they are converted to Quantity 1st
  • Added line_info.py module with the class to create line info widget

  • Renamed base.py which contained only shell info widget code to shell_info.py and similarly done this renaming in tests and __init__.py

  • Added util.py as a helper functions module to current widgets modules

    • create_table_widget was common in both line_info.py and shell_info.py, so removed it from shell_info.py and moved it there
    • Added a TableSummaryLabel class for creating summary label targetting a table widget (required for handling creation/updation of Total Packets label in last line counts table)
  • Added test_line_info.py for testing line info module. Note, there's no test file for util.py because it doesn't have any back-end logic but only front-end logic (of creating widgets) and since line_info module uses it so it's already covered.

To-Dos

  • Improvement in line info widget as described here
  • Add all the functions from notebook and make the OO interface work
  • Add docstrings in module
  • Create unit-tests
  • Add documentation (implemented in Add widgets documentation to tardis docs #1280 since it makes changes to entire widgets docs not only line info)

@jaladh-singhal jaladh-singhal requested review from MarkMageeAstro, wkerzendorf and ycamacho and removed request for wkerzendorf August 5, 2020 13:41
@jaladh-singhal jaladh-singhal force-pushed the widgets/line-info branch 2 times, most recently from 4a25a9a to e2615e9 Compare August 21, 2020 17:14
@codecov
Copy link

codecov bot commented Aug 21, 2020

Codecov Report

Merging #1264 into master will increase coverage by 1.20%.
The diff coverage is 95.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1264      +/-   ##
==========================================
+ Coverage   80.54%   81.75%   +1.20%     
==========================================
  Files          41       44       +3     
  Lines        3423     3699     +276     
==========================================
+ Hits         2757     3024     +267     
- Misses        666      675       +9     
Impacted Files Coverage Δ
tardis/widgets/util.py 85.36% <85.36%> (ø)
tardis/widgets/line_info.py 96.15% <96.15%> (ø)
tardis/widgets/shell_info.py 96.00% <100.00%> (ø)
tardis/widgets/tests/test_line_info.py 100.00% <100.00%> (ø)
tardis/widgets/tests/test_shell_info.py 100.00% <100.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 862025e...b54358b. Read the comment docs.

@jaladh-singhal jaladh-singhal marked this pull request as ready for review August 25, 2020 19:25
@epassaro
Copy link
Member

Noticed there are some Pandas ix method in analysis.py . This method is already deprecated and should be replaced by loc or iloc.

@jaladh-singhal
Copy link
Member Author

jaladh-singhal commented Aug 28, 2020

Noticed there are some Pandas ix method in analysis.py . This method is already deprecated and should be replaced by loc or iloc.

@epassaro Yes that file is very old, I'm actually using only very few methods of LastLineInteraction class from analysis.py in line_info.py and none of them are using ix. Now that you have mentioned it, perhaps we can scrap this file altogether and mimic only the functionality I require from it directly within the line_info.py!

I would like to know opinions of reviewers what do they think, should we do this change right here in this PR or get this merged first and then do the changes (to avoid unexpected surprises 😀 )?

@MarkMageeAstro
Copy link
Contributor

This is a pretty minor comment that likely doesn't affect many people, but I've updated my notebooks to have a dark background. This makes it difficult to read some of the text, as it's black on a dark background. This is shown in the attached image. Maybe adding a white background or changing the colour to something like red would fix this?

Screenshot 2020-09-09 at 16 36 27

@MarkMageeAstro
Copy link
Contributor

Some tests are failing. Looks like it's related to the integrated spectrum. Some other recent PRs aren't failing, so could need rebasing?

Copy link
Contributor

@MarkMageeAstro MarkMageeAstro left a comment

Choose a reason for hiding this comment

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

Some comments left for minor changes. Mostly about adding comments, etc. The comment I left about dark backgrounds shouldn't hold this PR back from being merged.

Copy link
Member

@jamesgillanders jamesgillanders left a comment

Choose a reason for hiding this comment

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

See comments above from myself and others - if these are addressed then I am happy to merge

Copy link
Contributor

@ycamacho ycamacho left a comment

Choose a reason for hiding this comment

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

This is a great PR @jaladh-singhal! We have finished our review of it and have asked for minor changes. Please look them over - if anything is taking too long to figure out we can make it an issue to fix for later. This PR is nearly ready to be merged!

tardis/widgets/line_info.py Outdated Show resolved Hide resolved
tardis/widgets/line_info.py Show resolved Hide resolved
tardis/widgets/line_info.py Outdated Show resolved Hide resolved
tardis/widgets/line_info.py Show resolved Hide resolved
Copy link
Contributor

@ycamacho ycamacho left a comment

Choose a reason for hiding this comment

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

Looks ready to merge to me!

Copy link
Contributor

@MarkMageeAstro MarkMageeAstro left a comment

Choose a reason for hiding this comment

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

Also looks good to me

@ycamacho ycamacho merged commit cc6fb1f into tardis-sn:master Sep 21, 2020
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
* Added line info code from notebook to a module

* Added filter by absorption frequency in LastLineInteraction

* Updated data methods and added more widgets

* Placed common function create_table_widget in util.py

* Added table sumary label widget

* Added line info event listeners and display

* Cleanup of irrelevant to-dos in base.py

* Renamed base.py to shell_info which is more relevant

* Also renamed test_base to test_shell_info

* Added docstrings to class methods

* Added docstrings to handler functons and some improvements

* Make sure that erg/s are always together in plot labels

* Import re for regex methods to work

* Make line info width fixed to prevent jumping output

* Fixed divide by zero warning in nu to lambda conversion

* Renamed functions & variables to make concepts clearer

* Added plotly and ipywidgets to environment file

* Add exception handling for invalid species

* Added test for data handling methods of line_info module

* Added tests for line info widget events

* Added test for change in group mode dropdown

* Renamed last_interaction_in_nu to input_nu in analysis.py

* Updated unit conversion to erg/s/A

* Added missing docstring for test_get_species_interactions

* Bring hidden scatter point in mid to remove extra space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

5 participants