adding RPacketPlotter to TARDIS

[jayantbhakar]

📝 Description

Type: 🚀 feature

This PR is based on the Montecarlo 2D packet visualization project. It aims to write a module for the RPacketPlotter tool which creates a 2D animated graph of the RPackets or the real packets used in the Montecarlo simulation.

Example code to plot is shown below:

rpacket_plotter = RPacketPlotter.from_simulation(sim, no_of_packets=10)
rpacket_plotter.generate_plot()

Here's how the plot looks like:

Light Theme:

Dark theme:

Also, link issues affected by this pull request by using the keywords: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

• Testing locally: Here is the log of pytests that I have run locally.

☑️ 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.

[codecov]

Codecov Report

Attention: Patch coverage is 98.54369% with 3 lines in your changes missing coverage. Please review.

Project coverage is 69.67%. Comparing base (e88a31a) to head (d83ceae).
Report is 1 commits behind head on master.

Files	Patch %	Lines
tardis/visualization/tools/rpacket_plot.py	96.73%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2119      +/-   ##
==========================================
+ Coverage   69.25%   69.67%   +0.42%     
==========================================
  Files         179      181       +2     
  Lines       14262    14468     +206     
==========================================
+ Hits         9877    10081     +204     
- Misses       4385     4387       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jaladh-singhal]

I'll review the code in the module soon. Please start writing unit tests and updating the docs

[jayantbhakar]

Yeah, I think before writing tests and docs, I should work on adding some functionalities that are left. Today I am working on the dark theme.

[MarkMageeAstro]

Nice work Jayant. This will be a really nice addition to TARDIS when it's merged. I have a few specific codes that I've left. Generally though I think it would be good to add more comments throughout the code -- comments never hurt. The docstrings are good, but there are some specific bits of code where it's not clear exactly what they're doing. Just adding some comments to explain that will really help

[jaladh-singhal]

Good to see things finally shaping up - the final plot looks great. Thanks for finishing up animation and adding themes.

I have few minor comments about code n comments

[andrewfullard]

Okay, I did the rebase so now it just needs some cleanup to the current code structure.

[jamesgillanders]

@jayantbhakar What is left for us to check/review with this PR? I haven't seen any updates since Andrew's rebasing -- did that resolve the issues you were facing, or is there more work needed?

[jayantbhakar]

I have done the required changes in the RPacket plotter code as per the new restructured code. Still I don't know why docs are failing?

[andrewfullard]

I have done the required changes in the RPacket plotter code as per the new restructured code. Still I don't know why docs are failing?

That's okay, it's because of changes to the workflow on master. There is still one merge conflict. If you merge master into the PR branch you should be able to resolve it.

[jayantbhakar]

@andrewfullard what do you mean by merging the master branch into my PR? I did git fetch upstream before pushing the changes to this PR. Doesn't that merge the upstream master into my branch? Or is it different? what command should i run for it?

[andrewfullard]

git fetch upstream just updates your local copy, it doesn't apply it to this branch. You need to do git merge master to bring the latest master branch into yours, which will result in at least one conflict.

[jayantbhakar]

I think everything is done.

[jayantbhakar]

@jaladh-singhal @sonachitchyan @jamesgillanders @MarkMageeAstro please review the PR.

[jamesgillanders]

Happy for this to now be merged.

Great work Jayant -- thanks for persevering with the PR and for all the recent work getting it to be compatible with the re-structured code base!

I really like how this tool has turned out, and it will certainly be a valuable resource for both demonstrating how the code works to new users, and for investigating potential bugs in certain model set ups.

I hope you're as happy with this tool -- and the code -- as we are!

[jamesgillanders]

@andrewfullard You're a GitHub guru, right? Why can't I merge this?

[andrewfullard]

@andrewfullard You're a GitHub guru, right? Why can't I merge this?

There are still 2 unaddressed changes that need to be cleared by the reviewers, but I can clear that.

Changes addressed

[jayantbhakar]

Happy for this to now be merged.

Great work Jayant -- thanks for persevering with the PR and for all the recent work getting it to be compatible with the re-structured code base!

I really like how this tool has turned out, and it will certainly be a valuable resource for both demonstrating how the code works to new users, and for investigating potential bugs in certain model set ups.

I hope you're as happy with this tool -- and the code -- as we are!

Same here! Delighted to see this PR get merged, finally, after around 2 years :)
And yes, the plot looks cool! Even a fella like me with no physics background can get a pretty good idea about what's going on here!

[jayantbhakar]

And thanks @andrewfullard for clearing up the changes, although they were addressed by me a long time ago, not sure why they were not marked resolved. But yeah, better late than never.