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, VIZ] updating draw() for using a graph layout that allows us to fix node positions #26

Merged
merged 27 commits into from
Dec 21, 2022

Conversation

siebert-julien
Copy link
Contributor

Fixes #24

Changes proposed in this pull request:

  • adding a pos argument to the draw function
  • adding an example for drawing graphs with fixed layout
  • adding a test for 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.

@siebert-julien siebert-julien force-pushed the draw_with_position_layout branch from ccf4afc to dcbb1cb Compare December 16, 2022 15:44
pywhy_graphs/viz/draw.py Outdated Show resolved Hide resolved
tests/test_draw.py Outdated Show resolved Hide resolved
tests/test_draw.py Outdated Show resolved Hide resolved
tests/test_draw.py Outdated Show resolved Hide resolved
tests/test_draw.py Outdated Show resolved Hide resolved
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.

Some minor edits. Great work and thanks for also writing up an example!

I pushed a commit to main to fix a CI issue. You probably will see some issues with formatting, which you can fix locally. You can check:

poetry run poe apply_format
poetry run poe lint
poetry run poe type_check
poetry run poe build_docs

For more information, see https://github.com/py-why/pywhy-graphs/blob/main/CONTRIBUTING.md

@siebert-julien
Copy link
Contributor Author

Hi @adam2392 I accepted the minor changes you requested.

I also run the formatting checks.

I had no problem with poetry run poe apply_format and poetry run poe type_check.

Unfortunately poetry run poe lint was taking ages, I had to kill it.

I hope everything is fine now.

@adam2392
Copy link
Collaborator

adam2392 commented Dec 21, 2022

Unfortunately poetry run poe lint was taking ages, I had to kill it.

This could be because not all the dev dependencies were installed (e.g. flake8, etc.). Can you try running:

poetry install --only style? I should add a check whether or not the libs are even installed. I think flake8 times out for some reason if you don't have it installed rather than raising an error... not sure why. Here is the output I get when running locally.

(pywhy-graphs) adam2392@Adams-MacBook-Pro pywhy-graphs % poetry run poe lint
Poe => flake8
./tests/test_draw.py:1:1: F401 'pywhy_graphs' imported but unused
./tests/test_draw.py:5:1: F401 'pywhy_graphs.CPDAG' imported but unused
./tests/test_draw.py:5:1: F401 'pywhy_graphs.PAG' imported but unused
./tests/test_draw.py:11:101: E501 line too long (111 > 100 characters)
./tests/test_draw.py:29:101: E501 line too long (131 > 100 characters)
./tests/test_draw.py:47:101: E501 line too long (105 > 100 characters)
./tests/test_draw.py:61:101: E501 line too long (131 > 100 characters)
./examples/draw_and_compare_graphs_with_same_layout.py:8:101: E501 line too long (114 > 100 characters)
./examples/draw_and_compare_graphs_with_same_layout.py:15:101: E501 line too long (106 > 100 characters)
./examples/draw_and_compare_graphs_with_same_layout.py:17:101: E501 line too long (119 > 100 characters)
./examples/draw_and_compare_graphs_with_same_layout.py:42:101: E501 line too long (102 > 100 characters)
./pywhy_graphs/viz/draw.py:16:101: E501 line too long (111 > 100 characters)
Poe => bandit -r pywhy_graphs -c pyproject.toml
[main]  INFO    profile include tests: None
[main]  INFO    profile exclude tests: None
[main]  INFO    cli include tests: None
[main]  INFO    cli exclude tests: None
[main]  INFO    using config: pyproject.toml
[main]  INFO    running on Python 3.9.13
Run started:2022-12-21 00:29:26.630414

Test results:
        No issues identified.

Code scanned:
        Total lines of code: 1785
        Total lines skipped (#nosec): 0

Run metrics:
        Total issues (by severity):
                Undefined: 0
                Low: 0
                Medium: 0
                High: 0
        Total issues (by confidence):
                Undefined: 0
                Low: 0
                Medium: 0
                High: 0
Files skipped (0):
Poe => codespell pywhy_graphs/ doc/ examples/ --ignore-words=.codespellignore --skip **/_build/*
examples/draw_and_compare_graphs_with_same_layout.py:42: softwares ==> software
Poe => pydocstyle .
Error: Subtasks _flake8, _codespell returned non-zero exit status 

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.

Almost there!

  • Can you add a changelog entry in docs/whats_new/v0.1.rst?
  • The CIs should work now and will show you the issues w/ the code formatting. Lmk if the poetry install of the Dev dependencies do not work well.

examples/draw_and_compare_graphs_with_same_layout.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2022

Codecov Report

Merging #26 (cb89339) into main (8c2ff2e) will increase coverage by 0.71%.
The diff coverage is 17.24%.

@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
+ Coverage   68.27%   68.99%   +0.71%     
==========================================
  Files          14       14              
  Lines         788      803      +15     
  Branches      208      213       +5     
==========================================
+ Hits          538      554      +16     
+ Misses        204      197       -7     
- Partials       46       52       +6     
Impacted Files Coverage Δ
pywhy_graphs/viz/draw.py 36.36% <17.24%> (+36.36%) ⬆️

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

siebert-julien and others added 14 commits December 21, 2022 21:20
…ix nodes positions while rendering py-why#24

Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Co-authored-by: Adam Li <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Co-authored-by: Adam Li <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Co-authored-by: Adam Li <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Co-authored-by: Adam Li <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Co-authored-by: Adam Li <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Co-authored-by: Adam Li <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Co-authored-by: Adam Li <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Co-authored-by: Adam Li <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Co-authored-by: Adam Li <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
dependabot bot and others added 3 commits December 21, 2022 21:20
Bumps [abatilo/actions-poetry](https://github.com/abatilo/actions-poetry) from 2.1.6 to 2.2.0.
- [Release notes](https://github.com/abatilo/actions-poetry/releases)
- [Changelog](https://github.com/abatilo/actions-poetry/blob/master/.releaserc)
- [Commits](abatilo/actions-poetry@2.1.6...v2.2.0)

---
updated-dependencies:
- dependency-name: abatilo/actions-poetry
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Julien Siebert <[email protected]>
@siebert-julien siebert-julien force-pushed the draw_with_position_layout branch from 6c45af7 to e72c524 Compare December 21, 2022 20:20
Signed-off-by: Julien Siebert <[email protected]>
@adam2392
Copy link
Collaborator

The CircleCI error: /home/circleci/project/docs/whats_new/v0.1.rst:34: WARNING: py:func reference target not found: pywhy_graphs.viz.draw can be fixed by adding draw function to the api.rst file.

There I would create another section for "Visualization of causal graphs" and you can follow the pattern shown for other functions to add draw somewhere there.

@siebert-julien
Copy link
Contributor Author

@adam2392 The CI fails on building the doc. I am out of idea why this is the case (at first I thought the URL syntax was the root cause, but I think I fixed that). Any idea?

Concerning the style dependencies, they were all installed properly. The problem seems to come from flake8 that does not respond for some reasons (I haven't investigated that).

@adam2392
Copy link
Collaborator

@adam2392 The CI fails on building the doc. I am out of idea why this is the case (at first I thought the URL syntax was the root cause, but I think I fixed that). Any idea?

Concerning the style dependencies, they were all installed properly. The problem seems to come from flake8 that does not respond for some reasons (I haven't investigated that).

No worries. I fixed it. See #26 (comment).

docs/whats_new/v0.1.rst Outdated Show resolved Hide resolved
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 @siebert-julien for the code contribution! Will merge once CI gives the green light.

@adam2392 adam2392 changed the title updating draw() for using a graph layout [ENH, VIZ] updating draw() for using a graph layout that allows us to fix node positions Dec 21, 2022
@adam2392 adam2392 merged commit b4bbebe into py-why:main Dec 21, 2022
adam2392 added a commit to adam2392/pywhy-graphs that referenced this pull request Dec 22, 2022
… fix node positions (py-why#26)

* updating draw() function for taking into account a graph layout and fix nodes positions while rendering
* Update contributing doc to show how to make changes
* Update examples/draw_and_compare_graphs_with_same_layout.py
* Bump abatilo/actions-poetry from 2.1.6 to 2.2.0 (py-why#27)

Bumps [abatilo/actions-poetry](https://github.com/abatilo/actions-poetry) from 2.1.6 to 2.2.0.
- [Release notes](https://github.com/abatilo/actions-poetry/releases)
- [Changelog](https://github.com/abatilo/actions-poetry/blob/master/.releaserc)
- [Commits](abatilo/actions-poetry@2.1.6...v2.2.0)

Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: Julien Siebert <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Adam Li <[email protected]>
Co-authored-by: Adam Li <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Adam Li <[email protected]>
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.

Ability to have layout of graph returned by viz.draw match that of another graph.
3 participants