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.

Sign up for GitHub

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

Jump to bottom

Adding Instructions For Enabling virtual_packet_logging Properties in Config #1551

Merged
merged 1 commit into from
May 14, 2021
Merged

Adding Instructions For Enabling virtual_packet_logging Properties in Config #1551

merged 1 commit into from
May 14, 2021

Conversation

DhruvSondhi
Copy link
Contributor

@DhruvSondhi DhruvSondhi commented Apr 25, 2021 •
edited
Loading

Adds the required instructions for enabling virtual_packet_logging via config
Fixes #1374

Description
This PR aims to add the required instructions for enabling the virual_packet_logging property in the Generating widget's notebook :)

Motivation and context
Adding these instructions would make it clear for the user to enable it for using the SDEC (Kromer) Plotting capabilities

How has this been tested?

  • Testing pipeline.
  • Other.

ScreenShot
image

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.

@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 Apr 25, 2021 •
edited
Loading

Codecov Report

Merging #1551 (dc6867e) into master (c384d74) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1551   +/-   ##
=======================================
  Coverage   68.24%   68.24%           
=======================================
  Files          73       73           
  Lines        6373     6373           
=======================================
  Hits         4349     4349           
  Misses       2024     2024

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 c384d74...dc6867e. Read the comment docs.

@DhruvSondhi DhruvSondhi force-pushed the instructions_sdec_plot branch 2 times, most recently from 0e44f7a to 4056385 Compare May 1, 2021 06:48
@DhruvSondhi DhruvSondhi force-pushed the instructions_sdec_plot branch from 4056385 to dc6867e Compare May 5, 2021 05:55
@andrewfullard andrewfullard merged commit 5166037 into tardis-sn:master May 14, 2021
@DhruvSondhi DhruvSondhi deleted the instructions_sdec_plot branch May 16, 2021 14:42
jaladh-singhal added a commit that referenced this pull request May 17, 2021
@jaladh-singhal
Revert "Adding Instructions for enabling virtual_packet_logging (#1551)"
3e18f1f 
This reverts commit 5166037.
jaladh-singhal
jaladh-singhal reviewed May 17, 2021
View reviewed changes
docs/using/visualization/generating_widgets.ipynb
@@ -575,11 +575,16 @@
]
Copy link
Member

@jaladh-singhal jaladh-singhal May 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This should not be placed on same level as that of Line Info Widget. Think about it - since we only need this instruction for line info widget it should be nested within it instead. Just remove the heading, it will automatically nest.

Also, correction: "... to show virtual packets' spectrum in Line Info Widget. Thus, ..." not SDEC Plot.


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.

Should I do something wrt to this change @jaladh-singhal ?

@jaladh-singhal
Copy link
Member

jaladh-singhal commented May 17, 2021 •
edited
Loading

Sorry for a very late review that it got merged - I would suggest creating a new PR following these comments, I've reverted this merge!

@jaladh-singhal jaladh-singhal mentioned this pull request May 17, 2021
Merged
jaladh-singhal added a commit that referenced this pull request May 17, 2021
@jaladh-singhal
Revert "Adding Instructions for enabling virtual_packet_logging (#1551)…
bf46269 
…" (#1574)

This reverts commit 5166037.
@DhruvSondhi
Copy link
Contributor Author

Sure, I will do it as soon as possible :)

@DhruvSondhi DhruvSondhi mentioned this pull request May 17, 2021
Merged
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
@DhruvSondhi
Adding Instructions for enabling virtual_packet_logging (tardis-sn#1551)
bdf6ce3
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
@jaladh-singhal
Revert "Adding Instructions for enabling virtual_packet_logging (tard…
c92410a 
…is-sn#1551)" (tardis-sn#1574)

This reverts commit 5166037.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaladh-singhal jaladh-singhal jaladh-singhal left review comments

@andrewfullard andrewfullard andrewfullard approved these changes

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add instruction to enable virt_packet_logging in widgets documentation
3 participants
@DhruvSondhi @jaladh-singhal @andrewfullard