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

[ENH] Add ability to draw graphs with a title #71

Merged
merged 10 commits into from
Apr 3, 2023

Conversation

aryan26roy
Copy link
Collaborator

Closes #23

Changes proposed in this pull request:

  • Adds a keyword argument for graph labels to viz.draw

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

After submitting

  • All GitHub Actions jobs for my pull request have passed.

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Copy link
Collaborator

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

Can you add a unit-test and also add a changelog entry to docs/whats_new/v0.1.rst?

@aryan26roy
Copy link
Collaborator Author

Sure! Just getting used to the procedure for this repo.

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
@adam2392 adam2392 changed the title Graph title [ENH] Add ability to draw graphs with a title Mar 30, 2023
Copy link
Collaborator

@adam2392 adam2392 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 the PR! It mostly looks good minus a few nitpicks.

One last thing: Can you also augment the existing example in examples/ to show off the label feature? E.g. in https://output.circle-artifacts.com/output/job/e94330e9-bd6c-4d83-ae14-8ed903121326/artifacts/0/dev/auto_examples/visualization/draw_and_compare_graphs_with_same_layout.html#sphx-glr-auto-examples-visualization-draw-and-compare-graphs-with-same-layout-py it would be great to see some sensible titles of the different graphs.

You can build the documentation locally to test it out and what it looks like by following the Contributing.md doc. Lmk if you have any issues! You'll also want to fix the coding style using the relevant poetry run poe apply_format command as well (which runs black and isort in the background)

docs/whats_new/v0.1.rst Outdated Show resolved Hide resolved
pywhy_graphs/viz/tests/test_draw.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2023

Codecov Report

Merging #71 (c2ce3eb) into main (c387f8a) will increase coverage by 0.38%.
The diff coverage is 71.49%.

@@            Coverage Diff             @@
##             main      #71      +/-   ##
==========================================
+ Coverage   84.03%   84.42%   +0.38%     
==========================================
  Files          29       34       +5     
  Lines        2255     2555     +300     
  Branches      578      687     +109     
==========================================
+ Hits         1895     2157     +262     
+ Misses        254      251       -3     
- Partials      106      147      +41     
Impacted Files Coverage Δ
pywhy_graphs/classes/admg.py 95.12% <ø> (ø)
pywhy_graphs/classes/cpdag.py 83.67% <ø> (ø)
pywhy_graphs/classes/pag.py 82.25% <ø> (ø)
pywhy_graphs/export/tigramite.py 0.00% <0.00%> (ø)
pywhy_graphs/testing.py 50.00% <50.00%> (ø)
pywhy_graphs/export/pcalg.py 55.04% <55.04%> (ø)
pywhy_graphs/export/tetrad.py 68.75% <68.75%> (ø)
pywhy_graphs/classes/intervention.py 88.17% <71.42%> (-3.72%) ⬇️
pywhy_graphs/export/numpy.py 72.85% <72.85%> (ø)
pywhy_graphs/export/causallearn.py 79.22% <80.95%> (ø)
... and 3 more

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

aryan26roy and others added 3 commits April 1, 2023 12:49
Co-authored-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Aryan Roy <50577809+aryan26roy@users.noreply.github.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
@aryan26roy
Copy link
Collaborator Author

Hey @adam2392 I think I implemented all the changes you requested. Sorry for the delay though.

@aryan26roy aryan26roy requested a review from adam2392 April 1, 2023 07:54
Copy link
Collaborator

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

LGTM once CI passes

@adam2392 adam2392 merged commit c1b4107 into py-why:main Apr 3, 2023
@adam2392
Copy link
Collaborator

adam2392 commented Apr 3, 2023

Thanks @aryan26roy !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to add title to viz.draw
3 participants