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

Added New Interactive Plasma Displaying Widget for Notebook Simulation #1640

Closed

Conversation

DhruvSondhi
Copy link
Contributor

@DhruvSondhi DhruvSondhi commented Jun 8, 2021

This PR aims to add a new interactive widget that can display the plasma stratification value (t_rad & w) for a particular iteration 🚀 . Allows to access values of all the iterations generated when running the simulation.

Description

This is feature request from @marxwillia, about the ability to see all the iteration values that were generated when running the simulation. Allows to infer the t_rad as well as w values.

Motivation and context

If the logger is set to log nothing, then the information cannot be inferred correctly. This widget aims to provide a way to access this without any changes to the code 😉

Screenshot

Wiget shows all the values for a particular iteration:
image

Wiget allows to select any iteration from the list & hence disply's value:
image

How has this been tested?

  • Testing pipeline.
  • Other.

Examples

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.

@tardis-bot
Copy link
Contributor

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.

1 similar comment
@tardis-bot
Copy link
Contributor

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.

@DhruvSondhi
Copy link
Contributor Author

We need to figure out how to display the widget on the notebook. display() doesn't seem to be the way to go but it works never the less. I will try to find an alternative way for the same 😄

tardis/simulation/base.py Outdated Show resolved Hide resolved
tardis/simulation/base.py Outdated Show resolved Hide resolved
tardis/simulation/base.py Outdated Show resolved Hide resolved
tardis/simulation/base.py Outdated Show resolved Hide resolved
tardis/visualization/widgets/shell_info.py Outdated Show resolved Hide resolved
@DhruvSondhi
Copy link
Contributor Author

Yes @andrewfullard , the # from tardis.simulation import Simulation part is important because I am actually using the create_table_widget() function present in the Util.py file. Because of this, it has circular imports. To address this situation, we need to remove this import or have it structured some other way.

@DhruvSondhi DhruvSondhi force-pushed the plasma_state_ipywidget branch 2 times, most recently from 900972b to a84186c Compare June 9, 2021 19:05
@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #1640 (456f3ac) into master (575002c) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1640      +/-   ##
==========================================
+ Coverage   58.42%   58.45%   +0.03%     
==========================================
  Files          66       66              
  Lines        6684     6706      +22     
==========================================
+ Hits         3905     3920      +15     
- Misses       2779     2786       +7     
Impacted Files Coverage Δ
tardis/tardis/simulation/base.py 81.85% <0.00%> (-1.40%) ⬇️

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 575002c...456f3ac. Read the comment docs.

@andrewfullard andrewfullard marked this pull request as draft June 14, 2021 15:24
@andrewfullard
Copy link
Contributor

Converting to draft for now.

@DhruvSondhi DhruvSondhi force-pushed the plasma_state_ipywidget branch 2 times, most recently from 2febe2e to b3f7cfd Compare June 15, 2021 06:25
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@DhruvSondhi DhruvSondhi force-pushed the plasma_state_ipywidget branch 2 times, most recently from e4a66c2 to 9844235 Compare August 1, 2021 09:17
@DhruvSondhi DhruvSondhi marked this pull request as ready for review August 21, 2021 07:42
@DhruvSondhi DhruvSondhi marked this pull request as draft August 22, 2021 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants