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.

Sign up for GitHub

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

Jump to bottom

The Node type should be an associated type of HugrView #1927

Closed
aborgna-q opened this issue Feb 24, 2025 · 0 comments · Fixed by #1932
Closed

The Node type should be an associated type of HugrView #1927

aborgna-q opened this issue Feb 24, 2025 · 0 comments · Fixed by #1932
Assignees
@lmondada

Comments

@aborgna-q
Copy link
Collaborator

Restricting the node indices to a specific type has worked well for us up until now, since we always reference nodes in the same internal base hugr (see #1926). This breaks down with more exotic implementations of HugrView that needs to track additional data on each index (see CQCL/tket2#778).

This proposal addresses that by making the Node index generic, defaulting to the current Node type for all the current impls. Citing @lmondada:

Node becomes an associated type in HugrView. The change will be verbose but should not impact users much. It shouldn't make our code significantly more verbose either, given that we can add all the required type bounds on the trait definition (Copy, Ord, Hash and conversions to/from portgraph).
@lmondada lmondada mentioned this issue Feb 24, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Feb 27, 2025
@lmondada
feat!: Add associated type Node to HugrView (#1932)
50139ff 
closes #1927

Yey, this is a fun one 🫠 

This makes `HugrView` general over the `Node` type. Note that `HugrMut`
and all operations that mutate hugrs still only act on the node type
`Node`.

I suggest once this is merged in that we add a `.git-blame-ignore-revs`
file to simplify future git blames:
```
# Commits to be ignored by git blame

# Add Node associated type to HugrView (large mechanical change)
<commit of this PR on main>
```

BREAKING CHANGE: `HugrView` now has a `Node` associated type. Make your
code generic over the node type where you can, replacing `Node` with
`H::Node`. Otherwise, use the type bound `H: HugrView<Node = Node>`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lmondada lmondada

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat!: Add associated type Node to HugrView CQCL/hugr
2 participants
@lmondada @aborgna-q