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

New layouts #360

Merged
merged 15 commits into from
May 15, 2023
Merged

New layouts #360

merged 15 commits into from
May 15, 2023

Conversation

thomasrobiglio
Copy link
Collaborator

As discussed in #359.
Added circular layout, spiral layout and barycenter_kamada_kawai_layout that does the same as barycenter_spring_layout but with a different cost function (implemented in networkx).

wrote explicitly the circular layout and the spiral layout.
added center in spiral
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Patch coverage: 98.50% and project coverage change: +0.09 🎉

Comparison is base (7b5cb1e) 90.49% compared to head (211d3af) 90.58%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #360      +/-   ##
==========================================
+ Coverage   90.49%   90.58%   +0.09%     
==========================================
  Files          41       41              
  Lines        3020     3060      +40     
==========================================
+ Hits         2733     2772      +39     
- Misses        287      288       +1     
Impacted Files Coverage Δ
xgi/drawing/layout.py 99.19% <98.50%> (-0.81%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@thomasrobiglio thomasrobiglio marked this pull request as draft May 12, 2023 13:32
@thomasrobiglio thomasrobiglio marked this pull request as ready for review May 12, 2023 13:47
xgi/drawing/layout.py Outdated Show resolved Hide resolved
xgi/drawing/layout.py Outdated Show resolved Hide resolved
xgi/drawing/layout.py Outdated Show resolved Hide resolved
xgi/drawing/layout.py Outdated Show resolved Hide resolved
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
xgi/drawing/layout.py Outdated Show resolved Hide resolved
xgi/drawing/layout.py Outdated Show resolved Hide resolved
xgi/drawing/layout.py Outdated Show resolved Hide resolved
xgi/drawing/layout.py Outdated Show resolved Hide resolved
xgi/drawing/layout.py Outdated Show resolved Hide resolved
xgi/drawing/layout.py Outdated Show resolved Hide resolved
@maximelucas
Copy link
Collaborator

This is great, thanks a lot Thomas.

Just let a few minor comments.
One thing missing: the new functions need to be added to the online docs (in this file https://github.com/xgi-org/xgi/blob/main/docs/source/api/drawing/xgi.drawing.layout.rst).

Could you also add the new functions to the plotting tutorial?

@thomasrobiglio
Copy link
Collaborator Author

Thank you max, i'll check all the comments in a bit. I did not put the new layouts in the tutorial because I was thinking we should open an issue to discuss how we want to re-organize the tutorial.

@maximelucas
Copy link
Collaborator

You're right we wanted to reorganise the tutorials. You can still add it now but otherwise we'll add it when we organise them no worries. Thank!!

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@thomasrobiglio
Copy link
Collaborator Author

@maximelucas I've just pushed the files with your comments (thx) + a simple cell in the drawing tutorial (tutorial 05). Let me know if I need to add other test to solve the ☂️ , else I'll let you merge the PR.

xgi/drawing/layout.py Outdated Show resolved Hide resolved
@maximelucas
Copy link
Collaborator

Thanks Thomas!
@leotrs you said you wanted to check this PR? If not, I'll merge after Thomas fixes my last comment.

+ minor fix in test (missing line in test_pairwise_spring_layout())
+ coherence in docstrings
+ run black
xgi/drawing/layout.py Outdated Show resolved Hide resolved
@maximelucas maximelucas merged commit 374a1ca into xgi-org:main May 15, 2023
@thomasrobiglio thomasrobiglio deleted the new_layouts branch May 15, 2023 14:19
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.

3 participants