Skip to content

Stop forcing edge order in edge_weights setter - #450

Merged
sgrava merged 6 commits into
mainfrom
sg/447-fix-edge-order-validation
Aug 14, 2026
Merged

sgrava merged 6 commits into
mainfrom
sg/447-fix-edge-order-validation

Conversation

@sgrava

@sgrava sgrava commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Fixes #447, #448.

Completely rewrite edge_weights.
Impl suffered from two issues:

  1. Imposing edge order when not needed. Undirected graph does not have preferred edge order.
  2. Setting weights from list is not really useful because edges does not have an intuitive order to follow like nodes. User likely need edges anyway to construct weights.

Changes

  • Rewrite edge_weights setter.
    • check that edges are the same independently on the order
    • update edges direcly. Arguably more direct and understandable then nx.set_edge_attribute(.., .. ,..)
    • improve docstring clarity.

Not fixed here

Leaves sorted_edges flying around.

  • Open an issue to completely remove sorting as unnecessary.

The setter validated input against sorted_edges, so a dict keyed with
the "wrong" (but equally valid, for an undirected graph) orientation
of an edge was rejected. Delegate straight to nx.set_edge_attributes,
which handles either orientation correctly, and drop the list-input
branch, which relied on the same unpredictable canonical order.

Fixes #447, #448.
@sgrava sgrava changed the title Stop forcing a canonical edge order in edge_weights setter Stop forcing edge order in edge_weights setter Aug 13, 2026
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-14 12:59 UTC

@sgrava sgrava added this to the v1.4 milestone Aug 14, 2026
@sgrava
sgrava requested a review from abussy-pasqal August 14, 2026 09:29

@abussy-pasqal abussy-pasqal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add in the BaseGraph doc whether the graph is directed or not ?

Comment thread qoolqit/graphs/base_graph.py
@sgrava

sgrava commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Can you add in the BaseGraph doc whether the graph is directed or not ?

Fair point, should be more clear now. Please, have a look.

@sgrava
sgrava requested a review from abussy-pasqal August 14, 2026 11:42
Comment thread qoolqit/graphs/base_graph.py
@sgrava
sgrava marked this pull request as ready for review August 14, 2026 12:41
@sgrava
sgrava requested a review from abussy-pasqal August 14, 2026 12:43
@sgrava
sgrava merged commit 9de6697 into main Aug 14, 2026
16 checks passed
@sgrava
sgrava deleted the sg/447-fix-edge-order-validation branch August 14, 2026 12:59
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.

edge_weights setter/getter rejects valid input due to forced edge-tuple ordering

2 participants