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

Zephyr layout coords fix #236

Merged
merged 10 commits into from
Nov 20, 2024

Conversation

mahdiehmalekian
Copy link

@mahdiehmalekian mahdiehmalekian commented May 22, 2024

A bug in zephyr_layout in the file dwave-networkx/drawing/zephyr_layout.py is fixed so that if scale = scale, all positions fit within [0, scale] on the x-axis and [-scale, 0] on the y-axis.

A couple of tests are added to tests/test_zephyr_layout.py that would fail before fixing the bug. These test the range of x value and y-value produced by zephyr_layout for the default scale=1 and scale=10.

Similarly, for pegasus_layout and test_pegasus_layout.py.

Closes #239.

Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 6.45161% with 29 lines in your changes missing coverage. Please review.

Project coverage is 75.05%. Comparing base (deb095f) to head (e0ccc6f).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
dwave_networkx/drawing/pegasus_layout.py 6.25% 15 Missing ⚠️
dwave_networkx/drawing/zephyr_layout.py 6.66% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #236      +/-   ##
==========================================
- Coverage   75.96%   75.05%   -0.91%     
==========================================
  Files          31       31              
  Lines        2184     2213      +29     
==========================================
+ Hits         1659     1661       +2     
- Misses        525      552      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@jackraymond
Copy link
Contributor

@randomir - can you assign myself and @AndyZzzZzzZzz as reviewers?

@randomir randomir requested a review from jackraymond October 22, 2024 14:40
@mahdiehmalekian
Copy link
Author

@randomir Is this ready to be merged?

Copy link
Contributor

@jackraymond jackraymond left a comment

Choose a reason for hiding this comment

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

I understand it would be standard if numpy was imported at the top rather than within the layout function. Double check with @randomir

range != [0,1] in layout as described. The update corrects this problem, unit tests are good.

It is not clear to me why scale=1 must imply [0,1], when I use pos in combination with draw_networkx (for example) plots look equally good before and after this pull request (to my eye, superficially). Is there a specific example of when this causes unexpected behaviour? Happy to assume there is one and approve the request though.

@mahdiehmalekian
Copy link
Author

You're correct that the plots don't look different before and after this PR. However the document says scale=1 gives values in [0,1], and this is a feature that was relied on in the layout algorithm (in dnx_layout in layout.py) and the bug in this code caused the layout algorithm to give poor results.

@mahdiehmalekian
Copy link
Author

@randomir
Could you advise on numpy import?

@randomir randomir linked an issue Nov 19, 2024 that may be closed by this pull request
Mahdieh Malekian and others added 3 commits November 19, 2024 20:10
Co-authored-by: Jack Raymond <10591246+jackraymond@users.noreply.github.com>
Co-authored-by: Andy Zhang <129815299+AndyZzzZzzZzz@users.noreply.github.com>
@randomir randomir merged commit 1631006 into dwavesystems:main Nov 20, 2024
19 checks passed
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.

pegasus_layout and zephyr_layout return out of range values
4 participants