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

Adding DataFrame for the RPacket Tracking Functionality #1776

Closed

Conversation

DhruvSondhi
Copy link
Contributor

@DhruvSondhi DhruvSondhi commented Aug 12, 2021

This is in continuation of the #1748
Adds the functionality of creating a dataframe from the interactions values of the packets tracked.
This PR will be rebased once #1748 is merged leading to addition of only a few commits.

Description

Motivation and context

How has this been tested?

  • Testing pipeline.
  • Other.

Examples

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.

Link to Documentation : https://dhruvsondhi.github.io/tardis/branch/packet_interaction_dataframe/io/output/rpacket_tracking.html

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@DhruvSondhi DhruvSondhi requested review from wkerzendorf, andrewfullard and Rodot- and removed request for wkerzendorf August 12, 2021 05:28
@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

@codecov
Copy link

codecov bot commented Aug 13, 2021

Codecov Report

Merging #1776 (e2598cb) into master (c705946) will decrease coverage by 5.14%.
The diff coverage is n/a.

❗ Current head e2598cb differs from pull request most recent head 6a4ad39. Consider uploading reports for the commit 6a4ad39 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1776      +/-   ##
==========================================
- Coverage   62.55%   57.40%   -5.15%     
==========================================
  Files          68       66       -2     
  Lines        7127     6846     -281     
==========================================
- Hits         4458     3930     -528     
- Misses       2669     2916     +247     
Impacted Files Coverage Δ
tardis/tardis/visualization/tools/sdec_plot.py 9.52% <0.00%> (-76.46%) ⬇️
...dis/tardis/montecarlo/montecarlo_numba/r_packet.py 25.43% <0.00%> (-19.01%) ⬇️
tardis/tardis/base.py 57.14% <0.00%> (-17.86%) ⬇️
tardis/tardis/montecarlo/montecarlo_numba/base.py 21.62% <0.00%> (-10.53%) ⬇️
tardis/tardis/visualization/plot_util.py 93.75% <0.00%> (-6.25%) ⬇️
.../montecarlo/montecarlo_numba/single_packet_loop.py 29.78% <0.00%> (-1.47%) ⬇️
...dis/montecarlo/montecarlo_numba/numba_interface.py 35.46% <0.00%> (-1.46%) ⬇️
...montecarlo/montecarlo_numba/calculate_distances.py 23.68% <0.00%> (-1.32%) ⬇️
...rdis/tardis/montecarlo/montecarlo_numba/vpacket.py 18.36% <0.00%> (-0.83%) ⬇️
tardis/tardis/model/base.py 88.29% <0.00%> (-0.67%) ⬇️
... and 5 more

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 c705946...6a4ad39. Read the comment docs.

@DhruvSondhi DhruvSondhi marked this pull request as ready for review August 16, 2021 15:35
@DhruvSondhi
Copy link
Contributor Author

DhruvSondhi commented Aug 16, 2021

This PR needs to be rebased after #1748 has been merged. And then it can be merged 🚀

@DhruvSondhi DhruvSondhi force-pushed the packet_interaction_dataframe branch from 41daaae to 2770626 Compare August 16, 2021 15:46
@DhruvSondhi DhruvSondhi force-pushed the packet_interaction_dataframe branch 5 times, most recently from bf179f4 to 78d91f6 Compare August 24, 2021 13:32
@DhruvSondhi DhruvSondhi marked this pull request as draft September 9, 2021 12:30
@DhruvSondhi DhruvSondhi marked this pull request as ready for review November 9, 2021 08:19
@DhruvSondhi DhruvSondhi force-pushed the packet_interaction_dataframe branch from 3eba913 to affffc3 Compare November 9, 2021 08:34
runner.rpacket_tracker = track_rpacket_dataframe(
runner.rpacket_tracker, tracked_df
)


@njit(**njit_dict)
def montecarlo_main_loop(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the docstring to include the returned tracks

@DhruvSondhi DhruvSondhi force-pushed the packet_interaction_dataframe branch from affffc3 to a593aec Compare December 3, 2021 04:50
@wkerzendorf wkerzendorf closed this Dec 8, 2021
Changed the implementation of the finalize_array function to reduce the array size
Implemented tests with Setup & Teardown mechanics for INITIAL_TRACKING_ARRAY_LENGTH
Removed track_r_packet func from r_packet.py
Instantiated RPacketCollection only when tracking is turned on,
Made it possible to store the packet interaction iteration wise
…ll the interaction,

Created new index with iterations,
Concatenated the dataframe for each individual iteration
Renamed functions & variables for some of the values
… pythonic way,

Appended Seed & Index value only once with this approach
@DhruvSondhi DhruvSondhi force-pushed the packet_interaction_dataframe branch from f404265 to ead38b5 Compare January 30, 2022 06:33
@andrewfullard andrewfullard marked this pull request as draft March 28, 2022 14:36
@andrewfullard
Copy link
Contributor

Closing, see #2073

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