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

[GSOC] Velocity Packet Tracker Visualization First Objective #2538

Closed
wants to merge 9 commits into from

Conversation

Sumit112192
Copy link
Contributor

@Sumit112192 Sumit112192 commented Mar 10, 2024

📝 Description

Type: 🎢 gsoc

This pull request contains my first objective approach of Velocity Packet Tracker Visualization.
It contains a jupyter .ipynb file and a folder containing the markdown files of the corresponding jupyter file.

📌 Resources

https://app.reviewnb.com/Sumit112192/tardis/blob/first-objective/VelocityPacketTrackerFirstObjective.ipynb/

Please let me know if the above link is not working.

🚦 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

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

*beep* *bop*

Hi, human.

I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently.

Please add your name and email to .mailmap in your current branch and push the changes to this pull request.

In case you need to map an existing alias, follow this example.

@Sumit112192
Copy link
Contributor Author

Sumit112192 commented Mar 10, 2024

@andrewfullard @wkerzendorf, Can anyone please review this? Thanks.

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently.

Please add your name and email to .mailmap in your current branch and push the changes to this pull request.

In case you need to map an existing alias, follow this example.

@@ -0,0 +1,3493 @@
{
Copy link
Member

@jaladh-singhal jaladh-singhal Mar 23, 2024

Choose a reason for hiding this comment

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

I don't think it's meaningful to show legend in this plot since we have no other color


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

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 submitting first objective @Sumit112192

I like the documentation of your thought process in your notebook. It will be nice to see the produced plots in plotly too (since our visualisation tools use both matplotlib and plotly).

Also, you didn't have to push markdown and images since we can see the rendered notebook through ReviewNB - feel free to take them out.

Looking forward to your proposal!

@Sumit112192
Copy link
Contributor Author

@jaladh-singhal Thanks for the review. I will make the changes that were suggested.

Copy link

codecov bot commented Mar 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.18%. Comparing base (1718002) to head (97ad956).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2538   +/-   ##
=======================================
  Coverage   68.18%   68.18%           
=======================================
  Files         168      168           
  Lines       14219    14219           
=======================================
  Hits         9695     9695           
  Misses       4524     4524           

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

Copy link

review-notebook-app bot commented Mar 29, 2024

View / edit / reply to this conversation on ReviewNB

jamesgillanders commented on 2024-03-29T15:31:32Z
----------------------------------------------------------------

Consider a different y-axis scale, since the less abundant elements are not well-visualised here


Copy link

review-notebook-app bot commented Mar 29, 2024

View / edit / reply to this conversation on ReviewNB

jamesgillanders commented on 2024-03-29T15:31:33Z
----------------------------------------------------------------

Consider a different y-axis scale


@jamesgillanders
Copy link
Member

Hi Sumit, looks good. Please take a look at the comments I left on the notebook

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 attached comments

@Sumit112192
Copy link
Contributor Author

@jamesgillanders I will make the changes as suggested. Also, will you please take a look at the doc version of my proposal, I have refined it.

@Sumit112192
Copy link
Contributor Author

@jamesgillanders How should I share the link of the doc version of my proposal?

@jamesgillanders
Copy link
Member

jamesgillanders commented Mar 29, 2024 via email

@Sumit112192 Sumit112192 deleted the first-objective branch September 8, 2024 13:08
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.

6 participants