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

Inconsistent OpTrait::other_{input,output} and OpType::static_{i,o}_port behaviour. #777

Closed
aborgna-q opened this issue Jan 4, 2024 · 0 comments · Fixed by #783
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@aborgna-q
Copy link
Collaborator

The Const operation defines its OpTrait::other_output as the EdgeKind::Static port with the const data.

hugr/src/ops/constant.rs

Lines 108 to 110 in 968c8b0

fn other_output(&self) -> Option<EdgeKind> {
Some(EdgeKind::Static(self.typ.clone()))
}

This port is then exposed by OpType::other_output_port.

On the other hand, OpType::static_output_port uses different logic to define the static output for Const;

hugr/src/ops.rs

Lines 196 to 201 in 968c8b0

/// If the op has a static output (Const, FuncDefn, FuncDecl), the port of that output.
pub fn static_output_port(&self) -> Option<OutgoingPort> {
OpTag::StaticOutput
.is_superset(self.tag())
.then_some(0.into())
}

This same situation happens with FuncDecl and FuncDef.

static_output_port is rarely used, so we haven't seen problems yet, but it makes the logic inconsistent.


If we compare the corresponding methods on the incoming side; we have Call and LoadConstant with hard-coded static inputs:

hugr/src/ops.rs

Lines 188 to 194 in 968c8b0

pub fn static_input_port(&self) -> Option<IncomingPort> {
match self {
OpType::Call(call) => Some(call.called_function_port()),
OpType::LoadConstant(l) => Some(l.constant_port()),
_ => None,
}
}

Both use DataflowOpTrait's default other_port; a StateOrder.

hugr/src/ops/dataflow.rs

Lines 20 to 22 in 968c8b0

fn other_input(&self) -> Option<EdgeKind> {
Some(EdgeKind::StateOrder)
}

static_input_port has multiple references in the code, where it gets used as a separate thing from other_input_port.


One would expect the same behaviour between input and output, so I consider this inconsistency a bug.
I'd propose changing the output_ports to mimic the input behaviour.

@aborgna-q aborgna-q added the bug Something isn't working label Jan 4, 2024
@aborgna-q aborgna-q self-assigned this Jan 4, 2024
@aborgna-q aborgna-q added this to the v0.1.0 milestone Jan 5, 2024
github-merge-queue bot pushed a commit that referenced this issue Jan 8, 2024
Closes #777.

Now `OpTrait` clearly separates `other_port` from `static_port`, and
`OpType` has a uniform interface for `value` ports (from the signature),
`static` ports (contstants), and `other` ports.

Fixes #779. The bug was caused by the encoder ignoring the constant
input port offset, so the decoder reconnected the edge to the
`StateOrder` port instead. Now we use the proper OpType methods.

Fixes #778.

---------

Co-authored-by: Seyon Sivarajah <seyon.sivarajah@quantinuum.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant