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

Last Interaction Velocity #2677

Merged
merged 26 commits into from
Jul 17, 2024

Conversation

sarthak-dv
Copy link
Contributor

@sarthak-dv sarthak-dv commented Jun 24, 2024

📝 Description

Type: 🚀 feature

This PR is in continuation of the PR #2651 which part of the Velocity Packet Tracker Visualisation tool. It introduces a new Python script interaction_radius_plot.py containing the InteractionRadiusPlotter class and a Jupyter notebook interaction_radius_plot.ipynb to demonstrate accessing the class and visualizing the plot.
The plots are generated using both Matplotlib and Plotly.

This PR aims to discuss and finalize the look and feel of the Last Interaction Radius plot.

Refer: #2651

Matplotlib:

output

Plotly:

newplot

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • My changes can't be tested (explain why)

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@tardis-bot
Copy link
Contributor

tardis-bot commented Jun 24, 2024

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

@sarthak-dv
Copy link
Contributor Author

sarthak-dv commented Jun 28, 2024

I have added a log_scale to the plot and a bins_range filter with which we can select the range of shells we want to plot:

Screenshot 2024-06-28 at 6 16 16 PM Screenshot 2024-06-28 at 6 21 31 PM

@jamesgillanders @MarkMageeAstro @jaladh-singhal Please review this, Thanks!

@sarthak-dv sarthak-dv force-pushed the line-interaction-radius branch from e80fc33 to 3b9a07f Compare June 29, 2024 16:41
@sarthak-dv sarthak-dv marked this pull request as ready for review July 2, 2024 14:12
Copy link
Member

@jaladh-singhal jaladh-singhal 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 coming along very nicely - thanks for adressing most of the past comments @sarthak-dv 🎉

I have few more comments to clean it up further. Besides that I don't see you adding your notebook to TOC. Also outputs need to be cleared. Please follow https://tardis-sn.github.io/tardis/contributing/development/documentation_guidelines.html which involves building docs locally to test if they appear ok and then making a docs-website build for us that we can test.

Overall, great job - I hope you are learning more about modularization and documentation through this.

tardis/transport/montecarlo/tests/test_numba_interface.py Outdated Show resolved Hide resolved
tardis/visualization/tools/interaction_radius_plot.py Outdated Show resolved Hide resolved
tardis/visualization/tools/interaction_radius_plot.py Outdated Show resolved Hide resolved
tardis/visualization/tools/interaction_radius_plot.py Outdated Show resolved Hide resolved
tardis/visualization/tools/interaction_radius_plot.py Outdated Show resolved Hide resolved
tardis/visualization/tools/interaction_radius_plot.py Outdated Show resolved Hide resolved
tardis/visualization/tools/interaction_radius_plot.py Outdated Show resolved Hide resolved
tardis/visualization/plot_util.py Show resolved Hide resolved
tardis/visualization/tools/sdec_plot.py Show resolved Hide resolved
tardis/visualization/tools/interaction_radius_plot.py Outdated Show resolved Hide resolved
docs/io/visualization/interaction_radius_plot.ipynb Outdated Show resolved Hide resolved
docs/io/visualization/interaction_radius_plot.ipynb Outdated Show resolved Hide resolved
docs/io/visualization/interaction_radius_plot.ipynb Outdated Show resolved Hide resolved
docs/io/visualization/interaction_radius_plot.ipynb Outdated Show resolved Hide resolved
docs/io/visualization/interaction_radius_plot.ipynb Outdated Show resolved Hide resolved
docs/io/visualization/interaction_radius_plot.ipynb Outdated Show resolved Hide resolved
docs/io/visualization/interaction_radius_plot.ipynb Outdated Show resolved Hide resolved
docs/io/visualization/interaction_radius_plot.ipynb Outdated Show resolved Hide resolved
docs/io/visualization/interaction_radius_plot.ipynb Outdated Show resolved Hide resolved
@sarthak-dv sarthak-dv force-pushed the line-interaction-radius branch 2 times, most recently from cb8c2a7 to 3911ddf Compare July 8, 2024 23:13
docs/io/visualization/how_to_liv_plot.ipynb Show resolved Hide resolved
docs/io/visualization/how_to_liv_plot.ipynb Show resolved Hide resolved
docs/io/visualization/how_to_liv_plot.ipynb Show resolved Hide resolved
docs/io/visualization/how_to_liv_plot.ipynb Show resolved Hide resolved
@@ -0,0 +1,419 @@
{
Copy link
Member

@jamesgillanders jamesgillanders Jul 9, 2024

Choose a reason for hiding this comment

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

Do we want to allow the user to specify a wider wavelength range than that covered by the model? Not sure -- let's discuss in our meeting...


Reply via ReviewNB

docs/io/visualization/how_to_liv_plot.ipynb Show resolved Hide resolved
@jamesgillanders
Copy link
Member

Hi Sarthak, great work so far!

I looked through the code and it looks good -- nice and concise, and clearly laid out.

I have added a bunch of comments to the notebook that came up when I was playing around with the tool. It would be good to get these additional modifications made to the tool -- I think they'll all improve it in some way. If any comments aren't clear, just send me a message and I can provide more detail. Or we can discuss in our meeting tomorrow.

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 my comments above

@sarthak-dv sarthak-dv force-pushed the line-interaction-radius branch 2 times, most recently from 6f630a9 to 046f3b8 Compare July 11, 2024 17:41
@andrewfullard andrewfullard enabled auto-merge (squash) July 12, 2024 14:25
@jaladh-singhal jaladh-singhal changed the title Last Interaction Radius Plot Last Interaction Velocity Plot Jul 12, 2024
jaladh-singhal
jaladh-singhal previously approved these changes Jul 12, 2024
Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my previous comments as well as what we discussed in the last meeting. I only have few minor wording related suggestion - not a dealbreaker though. Good to merge!

tardis/visualization/tools/liv_plot.py Outdated Show resolved Hide resolved
tardis/visualization/tools/liv_plot.py Outdated Show resolved Hide resolved
Comment on lines +295 to +296
if self._species_list is None or not self._species_list:
raise ValueError("No species provided for plotting.")
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is no longer the case since you updated the logic to handle None. But you know the edge cases better than me, so only remove it if you are sure

auto-merge was automatically disabled July 13, 2024 18:22

Head branch was pushed to by a user without write access

@sarthak-dv sarthak-dv force-pushed the line-interaction-radius branch from a83d4a6 to dbc3b09 Compare July 13, 2024 20:43
@andrewfullard andrewfullard enabled auto-merge (squash) July 15, 2024 13:52
@sarthak-dv sarthak-dv force-pushed the line-interaction-radius branch from bdd1b4b to 06e4fe8 Compare July 16, 2024 21:53
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.

This looks great @sarthak-dv! Really happy with it, and happy to merge!

@jamesgillanders jamesgillanders merged commit 94fccbe into tardis-sn:master Jul 17, 2024
10 checks passed
@sarthak-dv sarthak-dv changed the title Last Interaction Velocity Plot Last Interaction Velocity Aug 12, 2024
@sarthak-dv sarthak-dv deleted the line-interaction-radius branch August 22, 2024 21:12
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.

4 participants