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 docs for montecarlo packet visualization #2115

Merged

Conversation

jayantbhakar
Copy link
Member

📝 Description

Type: 📝 documentation

The documentation notebook has been added containing the basic plot for montecarlo packet visualization.

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

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Aug 10, 2022

Codecov Report

Merging #2115 (1e02ec2) into master (79f2fc3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2115   +/-   ##
=======================================
  Coverage   61.54%   61.54%           
=======================================
  Files          75       75           
  Lines        8635     8635           
=======================================
  Hits         5314     5314           
  Misses       3321     3321           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tardis-bot
Copy link
Contributor

tardis-bot commented Aug 10, 2022

*beep* *bop*

Hi, human.

The docs workflow has failed

Click here to see the build log.

@review-notebook-app
Copy link

review-notebook-app bot commented Aug 10, 2022

View / edit / reply to this conversation on ReviewNB

jaladh-singhal commented on 2022-08-10T20:25:59Z
----------------------------------------------------------------

In markdown, you need not to add trailing #s

And I think name we agreed on was "Montecarlo Visualization" (please feel free to ask if you forget). Not sure why you have "Single Packet_Ploty" mentioned?


jayantbhakar commented on 2022-08-11T10:50:32Z
----------------------------------------------------------------

I am sorry, I forgot to change it, I named it when I started working on this notebook.

@review-notebook-app
Copy link

review-notebook-app bot commented Aug 10, 2022

View / edit / reply to this conversation on ReviewNB

jaladh-singhal commented on 2022-08-10T20:25:59Z
----------------------------------------------------------------

Aren't convergence plots turned off by default?


jayantbhakar commented on 2022-08-11T10:52:00Z
----------------------------------------------------------------

This note came with the notebook I used as a reference. I will remove it.

@review-notebook-app
Copy link

review-notebook-app bot commented Aug 10, 2022

View / edit / reply to this conversation on ReviewNB

jaladh-singhal commented on 2022-08-10T20:26:00Z
----------------------------------------------------------------

Remove this from here


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 10, 2022

View / edit / reply to this conversation on ReviewNB

jaladh-singhal commented on 2022-08-10T20:26:01Z
----------------------------------------------------------------

All imports should be at the top together in a single cell


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 10, 2022

View / edit / reply to this conversation on ReviewNB

jaladh-singhal commented on 2022-08-10T20:26:02Z
----------------------------------------------------------------

The heading can't be H1, there should be only H1 per notebook/rst. Make it H2.

And IMO "Finding packet coordinates" is enough as heading - mathematical expression is implied in the details


jayantbhakar commented on 2022-08-11T10:56:07Z
----------------------------------------------------------------

this was h3 heading and not h1

@review-notebook-app
Copy link

review-notebook-app bot commented Aug 10, 2022

View / edit / reply to this conversation on ReviewNB

jaladh-singhal commented on 2022-08-10T20:26:03Z
----------------------------------------------------------------

Should add H2 before this cell something like "Plotting packet trajectories"


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 such a quick PR @jayantbhakar - I have few requests about organisation in notebook (see my comments on ReviewNB).

Other than that, why doesn't the final plot show up on https://tardis-sn.github.io/tardis/pull/2115/io/visualization/montecarlo_packet_visualization.html ? Please check code behind SDEC plotly plot that renders perfectly on docs website and see what you are missing in your code?

@isaacgsmith isaacgsmith self-requested a review August 11, 2022 04:49
Copy link
Member Author

I am sorry, I forgot to change it, I named it when I started working on this notebook.


View entire conversation on ReviewNB

Copy link
Member Author

This note came with the notebook I used as a reference. I will remove it.


View entire conversation on ReviewNB

Copy link
Member Author

this was h3 heading and not h1


View entire conversation on ReviewNB

jaladh-singhal
jaladh-singhal previously approved these changes Aug 11, 2022
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 comments.

Just one thing: the plot looks vertically stretched like an ellipse - I think we can fix that (make it look perfectly circular) by constraining the figure size (need not to be done in this, hence approving).

@jaladh-singhal jaladh-singhal enabled auto-merge (squash) August 19, 2022 15:16
@jaladh-singhal jaladh-singhal merged commit a8a9540 into tardis-sn:master Aug 19, 2022
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