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

fix: make PlanarEmbedding.copy() use add_edges_from() from parent (closes #7223) #7224

Merged
merged 1 commit into from Feb 28, 2024

Conversation

mdealencar
Copy link
Contributor

closes #7223

PlanarEmbedding inherits the method copy() from nx.Graph. However, the current implementation uses the method add_edges_from() which was made forbidden by PR #6798.

Current Behavior

embedding = nx.PlanarEmbedding({1: {2: {"cw": 2, "ccw": 2}},
                                2: {1: {"cw": 1, "ccw": 1}}})
emb2 = embedding.copy()
NotImplementedError: Use `add_half_edge` method to add edges to a PlanarEmbedding.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

To me, the real question is: do we want to support copying of PlanarEmbedding objects? I can't see any reason why not, so I don't object to making the copy method work.

It'd be nice to have a couple tests if for no other reason than to catch regressions, but it's not a blocker since this is technically just re-enabling behavior that was already "working" prior to #6798 !

Thanks @mdealencar !

@MridulS MridulS merged commit f5ada3f into networkx:main Feb 28, 2024
39 of 40 checks passed
@jarrodmillman jarrodmillman added this to the 3.3 milestone Feb 28, 2024
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
…closes networkx#7223) (networkx#7224)

fix: make `PlanarEmbedding.copy()` use `add_edges_from()` from parent class (planarity.py)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

PlanarEmbedding.copy() raising NotImplementedError after PR #6798
4 participants