-
Notifications
You must be signed in to change notification settings - Fork 6
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 #51
Conversation
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.
Well who knows maybe we'll need another hugr release to get there....but I hope this helps.
(First TKET2 PR I've looked at so I'm not sure I've earnt "Approve" button rights yet ;-) )
let sig = | ||
Signature::new_linear([vec![QB; num_qubits], vec![LINEAR_BIT; num_bits]].concat()); | ||
let sig = AbstractSignature::new_linear( | ||
[vec![QB; num_qubits], vec![LINEAR_BIT.clone(); num_bits]].concat(), |
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.
A bit puzzled as to why you now need to clone()
LINEAR_BIT
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.
LINEAR_BIT
is now initialized with lazy_static instead of defined as a const
. That requires some boilerplate uses like this.
Note that vec!
was doing some cloning anyways.
Some(c) => { | ||
let const_type = hugr::types::ClassicType::F64; | ||
assert!(c.check_type(&const_type).is_ok()); | ||
let const_op = Const::new(c, const_type).unwrap(); |
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.
This makes the check in the previous line for you (that's the failure that the unwrap()
is about), so you might simplify this
src/resource.rs
Outdated
@@ -25,20 +25,35 @@ pub const LINEAR_BIT_NAME: SmolStr = SmolStr::new_inline("LBit"); | |||
pub const JSON_OP_NAME: SmolStr = SmolStr::new_inline("TKET1 Json Op"); | |||
|
|||
lazy_static! { | |||
|
|||
/// The type for linear bits. Part of the TKET1 resource. | |||
pub static ref LINEAR_BIT: SimpleType = SimpleType::Qontainer(Container::Opaque(CustomType::new( |
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 can now (following CustomType's are Simple not necessarily Classic, and report their TypeTag hugr#328) "just" do
CustomType::new(...).into()
here - I guess there is no chance of changing that
static ref
intoconst
and then avoiding all the.clone()
. Hmmm.CustomType::new
really doesn't do anything much at all, so I'd be happy to see a change on the Hugr side to enable such....? - If (2.) doesn't work, then the other way would be to do
TKET1_RESOURCE.types().get(LINEAR_BIT_NAME).unwrap().instantiate_concrete(&[])
which would reduce the chance of getting anything inconsistent and is more "best practice"
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.
- Yeah, before we had a const
CustomType::new_simple
that didn't take arguments, but it was removed in Interactions between resource definitions and instances hugr#315
src/resource.rs
Outdated
|
||
let json_op_param = TypeParam::Value(HashableType::Container(Container::Opaque( | ||
CustomType::new("TKET1 Json Op", vec![], TKET1_RESOURCE_ID, TypeTag::Simple), |
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.
Really this type should be defined in the resource too, just like LINEAR_BIT
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.
Linear bit is a TypeDef
(with a corresponding SimpleType
.
This is a build-time operation TypeParam
(with a corresponding TypeArg
). How can I add it to a resource?
src/resource.rs
Outdated
@@ -56,23 +71,21 @@ pub(crate) fn wrap_json_op(op: &JsonOp) -> ExternalOp { | |||
TKET1_RESOURCE_ID, | |||
JSON_OP_NAME, | |||
"".into(), | |||
vec![TypeArg::Value(op)], | |||
Some(sig), | |||
vec![TypeArg::CustomValue(op)], |
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.
The OpaqueOp containing this should now be produced by TKET1_RESOURCE.operations().get(JSON_OP_NAME).unwrap().instantiate_concrete(vec![TypeArg::CustomValue(op)])
and I think that should even calculate the signature for you (assuming json_op_signature
does what I think it should)
src/resource.rs
Outdated
fn json_op_signature( | ||
args: &[TypeArg], | ||
) -> Result<(TypeRow<SimpleType>, TypeRow<SimpleType>, ResourceSet), SignatureError> { | ||
let [TypeArg::CustomValue(arg)] = args else { |
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, panicking here seems fine, the correctness of the &[TypeArg]
against the declared TypeParam's should already have been checked
src/resource.rs
Outdated
panic!("Wrong number of arguments"); | ||
// TODO: Add more Signature Errors | ||
//return Err(SignatureError::WrongNumArgs(1, args.len())); | ||
}; | ||
let op: JsonOp = serde_yaml::from_value(arg.clone()).unwrap(); // TODO Errors! | ||
let sig = op.signature(); | ||
Ok((sig.input, sig.output, sig.output_resources)) | ||
let Signature { |
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.
Hmmmm, this should probably become an AbstractSignature
and then you can use the resource_reqs
there, input_resources
is not really right here
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.
Changed this along with JsonOp::signature
src/json/op.rs
Outdated
let signature = AbstractSignature::new_df([linear.clone(), params].concat(), linear); | ||
Signature { | ||
signature, | ||
input_resources: ResourceSet::singleton(&TKET1_RESOURCE_ID), |
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.
This may become restricting. Basically you are saying that the inputs to whatever operation this is, will have needed TKET1_RESOURCE_ID
to compute them. Hmmm. This is in impl JsonOp
right? I think you should return an AbstractSignature
instead, with resource_reqs
being this singleton-set. Does that work?
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 added the resource to the output instead using AbstractSignature::with_resource_delta
.
Now the tests throw an error:
InvalidHUGR(
SrcExceedsTgtResources {
from: Node { index: NodeIndex(3) },
from_offset: Port { offset: Outgoing(0) },
from_resources: ResourceSet({"TKET1"}),
to: Node { index: NodeIndex(2) },
to_offset: Port { offset: Incoming(0) },
to_resources: ResourceSet({})
}
)
(rendered before connecting the edges to the output node)
I'll try to debug it.
tket-json-rs = { git = "https://github.com/CQCL/tket-json-rs", rev="619db15d3", features = [ |
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.
Hmmmm, is this a prerequisite, a driveby, a follow-up? (All the changes I see are hugr-related AFAICT.)
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.
Ah, it would be a small drive-by.
This makes sure the dependency here is the same as in the main Cargo.toml
.
Subsumed by #53 |
No description provided.