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

Add four simple layouts #310

Merged
merged 32 commits into from
May 31, 2021
Merged

Add four simple layouts #310

merged 32 commits into from
May 31, 2021

Conversation

Chriscrosser3310
Copy link
Contributor

Related to #280. Four simplest layouts were added: bipartite, circular, shell, and spiral. A few things to note:

  1. The rescale and recenter functions are duplicated with the ones in Add spring layout #306, we only need to keep one of each.
  2. The layout functions return the new type Pos2DMapping added in Add random_layout function and Pos2DMapping #305.
  3. The tests are performed as follows: generate a position dictionary from a layout function in networkx, and compare it to that generated by retworkx with the same layout function and inputs. If for all nodes, the difference between positions of the same nodes is smaller than some threshold (I chose 1E-6), the tests pass.

@coveralls
Copy link

coveralls commented Apr 23, 2021

Pull Request Test Coverage Report for Build 893561305

  • 278 of 282 (98.58%) changed or added relevant lines in 2 files are covered.
  • 18 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+1.09%) to 96.865%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/layout.rs 178 182 97.8%
Files with Coverage Reduction New Missed Lines %
src/lib.rs 18 99.32%
Totals Coverage Status
Change from base Build 888019140: 1.09%
Covered Lines: 8714
Relevant Lines: 8996

💛 - Coveralls

@mtreinish mtreinish added this to the 0.9.0 milestone Apr 23, 2021
Copy link
Collaborator

@georgios-ts georgios-ts left a comment

Choose a reason for hiding this comment

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

Great work!
My only comment is that you should consider updating all layout functions to handle graphs with holes in node indices.

retworkx/__init__.py Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/layouts.rs Outdated Show resolved Hide resolved
src/layouts.rs Outdated Show resolved Hide resolved
Chriscrosser3310 and others added 2 commits May 27, 2021 18:45
Co-authored-by: georgios-ts <45130028+georgios-ts@users.noreply.github.com>
Co-authored-by: georgios-ts <45130028+georgios-ts@users.noreply.github.com>
@Chriscrosser3310
Copy link
Contributor Author

Great work!
My only comment is that you should consider updating all layout functions to handle graphs with holes in node indices.

Thanks, this is a good point, I forgot that indices can be non-consecutive. Will update soon

Copy link
Collaborator

@georgios-ts georgios-ts left a comment

Choose a reason for hiding this comment

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

This

n = 5
graph = rx.generators.mesh_graph(n)
graph.remove_node(2)
rx.shell_layout(graph, nlist=[graph.node_indexes()])

will raise a panick exception.
I suggested some changes to fix this but feel free to proceed in your own way.

src/layout.rs Outdated Show resolved Hide resolved
src/layout.rs Outdated Show resolved Hide resolved
src/layout.rs Outdated Show resolved Hide resolved
src/layout.rs Show resolved Hide resolved
Chriscrosser3310 and others added 3 commits May 28, 2021 12:58
Co-authored-by: georgios-ts <45130028+georgios-ts@users.noreply.github.com>
Copy link
Collaborator

@georgios-ts georgios-ts left a comment

Choose a reason for hiding this comment

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

Great! One comment left and then it should be ready to go

src/layout.rs Outdated
Comment on lines 494 to 499

if let Some(scale) = scale {
rescale(&mut pos, scale, (0..node_num).collect());
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if let Some(scale) = scale {
rescale(&mut pos, scale, (0..node_num).collect());
}
}
}
if let Some(scale) = scale {
rescale(&mut pos, scale, (0..node_num).collect());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, I forgot there's an else if -- but the problem of moving it out is that if the graph only has 1 node, lim in rescale will be 0.0 and the point will become [NaN, NaN]. I guess I can modify rescale so it only does something when lim > 0.0.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Overall this LGTM, great work! I have 2 nits inline but not really worth blocking over. I might just update the test one myself real quickly so we can merge this (because getting good error messages from the tests are useful).

tests/digraph/test_layout.py Outdated Show resolved Hide resolved
Comment on lines +4292 to +4301
#[pyfunction]
#[text_signature = "(graph, /, scale=1, center=None, resolution=0.35,
equidistant=False)"]
pub fn digraph_spiral_layout(
graph: &digraph::PyDiGraph,
scale: Option<f64>,
center: Option<layout::Point>,
resolution: Option<f64>,
equidistant: Option<bool>,
) -> Pos2DMapping {
Copy link
Member

Choose a reason for hiding this comment

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

Personally I would do something like:

Suggested change
#[pyfunction]
#[text_signature = "(graph, /, scale=1, center=None, resolution=0.35,
equidistant=False)"]
pub fn digraph_spiral_layout(
graph: &digraph::PyDiGraph,
scale: Option<f64>,
center: Option<layout::Point>,
resolution: Option<f64>,
equidistant: Option<bool>,
) -> Pos2DMapping {
#[pyfunction(scale="1.0", resolution="0.35", equidistant="false")]
#[text_signature = "(graph, /, scale=1, center=None, resolution=0.35,
equidistant=False)"]
pub fn digraph_spiral_layout(
graph: &digraph::PyDiGraph,
scale: f64,
center: Option<layout::Point>,
resolution: f64,
equidistant: bool,
) -> Pos2DMapping {

so we set the default in the macro generated python c api ffi function. But this also works fine

This commit adds a new custom assertion method for comparing layouts
with a tolerance (fixed at 1e-6) ensuring that no layout differs from
the expected in any coordinate by more than this. If there is a failure
it will print a detailed message about which node differs from the
expected. With this change locally 2 bipartite layout tests failed so
this commit updates the expected values so it passes (the layouts still
look like valid bipartite layouts just the center point was different).
@mtreinish mtreinish merged commit ab1f984 into Qiskit:main May 31, 2021
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.

None yet

4 participants