-
Notifications
You must be signed in to change notification settings - Fork 7
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
chore: update hugr dependency #219
Conversation
Codecov Report
@@ Coverage Diff @@
## main #219 +/- ##
==========================================
+ Coverage 74.98% 75.06% +0.08%
==========================================
Files 30 30
Lines 4018 4039 +21
Branches 4018 4039 +21
==========================================
+ Hits 3013 3032 +19
- Misses 825 827 +2
Partials 180 180
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
@@ -155,7 +155,7 @@ impl Chunk { | |||
ConnectionTarget::TransitiveConnection(input_connection) | |||
} else { | |||
// Translate the original chunk node into the inserted node. | |||
(*node_map.get(&node).unwrap(), port).into() | |||
ConnectionTarget::InsertedOutput(*node_map.get(&node).unwrap(), port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could leave this line unchanged, right, as you are #[derive(...,From)]
on ConnectionTarget and #[from]
on Inserted(In,Out)put? I'd either use the From derivations or not derive them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropped the From, the explicit version is more clear
let edge_prop = | ||
PEdge::try_from_port(cmd.node(), in_offset, circuit).expect("Invalid HUGR"); | ||
let in_offset: IncomingPort = in_offset.into(); | ||
let edge_prop = PEdge::try_from_port(cmd.node(), in_offset.into(), circuit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do this more directly (avoiding in_offset
) via Port::new_incoming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use in_offset
below for linked_outputs
, so we need the Incoming port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. (Personal taste but I am starting to like var = Foo::from(x)
rather than var:Foo = x.into()
)
tket2/src/portmatching/pattern.rs
Outdated
(cx_gate, NodeID::CopyNode(inp, Port::new_outgoing(1))), | ||
( | ||
cx_gate, | ||
NodeID::CopyNode(inp, Port::new(Direction::Outgoing, 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NodeID::CopyNode(inp, Port::new(Direction::Outgoing, 0)) | |
NodeID::CopyNode(inp, Port::new_outgoing(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't exist anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I just found that myself, sorry. OutgoingPort::from(0).into()
, hmmm. Add NodeID::new_copy(Node, impl Into<Port>)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @aborgna-q !
This required a partial move from using
Port
everywhere to the more specificOutgoingPort
andIncomingPort
.I'll open a separate issue for the more pervasive changes like updating the
Units
iterator and theportmatching
module.For now, we just cast and
unwrap
in those cases.Requires CQCL/hugr#647