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

Fixing Plasma Graph to Allow .dot and .tex file Generation #1868

Merged
merged 10 commits into from
Feb 16, 2022

Conversation

bartnikm
Copy link
Contributor

@bartnikm bartnikm commented Jan 19, 2022

Description

This reworks the code necessary to generate the .dot and .tex files for the plasma graph. The documentation regarding the graph has been changed to reflect this.

Motivation and context

The current way TARDIS builds the .dot file makes it raise errors upon attempting to build the .tex file, and the latex code within the .dot file, if written into a .tex file without using TARDIS, is displayed incorrectly.

How has this been tested?

  • Testing pipeline.
  • Other.

Examples

https://bartnikm.github.io/tardis-bartnikm/branch/plasma-graph/physics/setup/plasma/index.html#the-plasma-graph

Below are two graphs of the same configuration file. The first one had no entries passed into the args parameter:
Sample_Graph_no_args.pdf

The second one had the following passed:
[r"nodesep=1.0", r'edge[lblstyle="fill=white"]', r'margin=0', r'ratio="fill"', r'size="8.3,11.7!"']
Sample_Graph_with_args.pdf

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.

…fully and to allow graphs to be displayed within a notebook; rewrote documentation to reflect this [build docs]
@codecov
Copy link

codecov bot commented Jan 19, 2022

Codecov Report

Merging #1868 (5209738) into master (3471658) will increase coverage by 0.22%.
The diff coverage is n/a.

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1868      +/-   ##
==========================================
+ Coverage   62.12%   62.34%   +0.22%     
==========================================
  Files          66       68       +2     
  Lines        6809     7149     +340     
==========================================
+ Hits         4230     4457     +227     
- Misses       2579     2692     +113     
Impacted Files Coverage Δ
tardis/tardis/plasma/base.py 48.63% <0.00%> (-7.27%) ⬇️
tardis/tardis/io/parsers/arepo.py 69.04% <0.00%> (ø)
.../montecarlo/montecarlo_numba/r_packet_transport.py 16.45% <0.00%> (ø)
tardis/tardis/util/base.py 72.31% <0.00%> (+0.11%) ⬆️
tardis/tardis/model/base.py 88.96% <0.00%> (+0.66%) ⬆️
...rdis/tardis/montecarlo/montecarlo_numba/vpacket.py 19.19% <0.00%> (+0.82%) ⬆️
...montecarlo/montecarlo_numba/calculate_distances.py 25.00% <0.00%> (+1.31%) ⬆️
...dis/montecarlo/montecarlo_numba/numba_interface.py 36.92% <0.00%> (+1.45%) ⬆️
.../montecarlo/montecarlo_numba/single_packet_loop.py 31.25% <0.00%> (+1.46%) ⬆️
...dis/tardis/montecarlo/montecarlo_numba/r_packet.py 44.44% <0.00%> (+19.00%) ⬆️

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 3471658...6c53e09. Read the comment docs.

@wkerzendorf
Copy link
Member

That looks interesting. Just a quick reminder - one PR one idea. In this PR there is the idea of fixing the old version and adding a new way of plotting this. Let's make a PR that fixes the old version first. Then we make a new PR that has the display_graph

…d randomized scaling factor to edges to display graph in a more readable format
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@bartnikm bartnikm marked this pull request as ready for review January 26, 2022 21:30
tardis/plasma/base.py Outdated Show resolved Hide resolved
tardis/plasma/base.py Outdated Show resolved Hide resolved
tardis/plasma/base.py Outdated Show resolved Hide resolved
@bartnikm
Copy link
Contributor Author

bartnikm commented Feb 2, 2022

The failed black test flagged multiple files which I did not edit nor are a part of this pull request

@isaacgsmith isaacgsmith mentioned this pull request Feb 8, 2022
10 tasks
@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.

tardis/plasma/base.py Outdated Show resolved Hide resolved
tardis/plasma/base.py Outdated Show resolved Hide resolved
tardis/plasma/base.py Outdated Show resolved Hide resolved
tardis/plasma/base.py Outdated Show resolved Hide resolved
@bartnikm bartnikm self-assigned this Feb 11, 2022
@bartnikm bartnikm changed the title Fixing Plasma Graph to Allow Automatic .dot and .tex file Generation Fixing Plasma Graph to Allow .dot and .tex file Generation Feb 12, 2022
…ers for displaying graphs with changable parameter [build docs]
@bartnikm bartnikm mentioned this pull request Feb 16, 2022
2 tasks
@wkerzendorf wkerzendorf merged commit 24d3322 into tardis-sn:master Feb 16, 2022
@bartnikm bartnikm deleted the plasma-graph branch February 17, 2022 20:43
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.

4 participants