[RasmitDevkota]

Description

Implements support for Var, Unary, and Binary operations in _append_if_else_circuit during qiskit_to_tk conversion.

Related issues

Closes #514

[CalMacCQ]

Thanks a lot for the PR :)

We can take a detailed look shortly.

A couple things

1. There are some linting issues here Most of these should be easy to address. If its not possible to avoid the private attribute access then you can add a NOQA comments as is done here.
2. Could you please add a test which would have failed before this change but now passes? You can add this to qiskit_convert_test.py. Maybe the snippet you showed in the issue would be a good building block for a test.

[RasmitDevkota]

Pushed fixes, a few comments:

• I'm not sure what would be the best resolution for the PLR0912 Too many branches (15 > 12) error, especially when Qiskit IfElseOps really do have this many potential forms.
• It doesn't seem possible to accept the bit register name or index without either having the original QuantumCircuit object which the bits are a part of (which the _append_if_else_circuit method does not have access to, at the moment, without changing the function signature) or using the private properties I am currently using. After going through the Qiskit source, I am somewhat surprised that there isn't a public property or method to get those values, but someone might be able to correct me!

[CalMacCQ]

Regarding the lints I think that ruff check . --fix should sort out the rest? I could be wrong.

[CalMacCQ]

Minor thing but we could add a type annotation for _condition and a return type annotation for flatten_condition

[RasmitDevkota]

Added!

[RasmitDevkota]

Looks like I had to ask ruff to do extra checks to get through the rest of the lint errors, but I think that should cover it.

[CalMacCQ]

I think after reformatting the NOQA comments are now on the wrong line which is kinda annyoying.

Would also be nice to have a test in qiskit_convert_test.py to demonstrate the fix and guard against regressions with future changes to the conversion code.

[RasmitDevkota]

I think after reformatting the NOQA comments are now on the wrong line which is kinda annyoying.

Yeah, I did notice that some of the formatting decisions didn't make sense, lie how it splits into a newline here:

return flatten_condition(_condition.left) ^ flatten_condition(
    _condition.right
)

Should I leave it this way, or make it more consistent? I've added the noqa lines to the private property lint errors, though!

Would also be nice to have a test in qiskit_convert_test.py to demonstrate the fix and guard against regressions with future changes to the conversion code.

Working on that now, would the standard structure be to test each of the different types of conditions separately, as in having a separate test for Var, Unary, and Binary, or something more compact? I have examples which reach all of the new branches I've created but I'm just not sure about organization.

[CalMacCQ]

Working on that now, would the standard structure be to test each of the different types of conditions separately, as in having a separate test for Var, Unary, and Binary, or something more compact? I have examples which reach all of the new branches I've created but I'm just not sure about organization.

Regarding the structure of the tests I don't have a strong opinion really. It would be desirable if the behaviour for Var, Unary and Binary was tested. Apart from that, feel free to write the test in a way that you're comfortable with and one of us can review and provide feedback if needed.

Also spotted that the linting is fixed now, thanks!

[CalMacCQ]

Happy to help with this if needed. Thanks again for the contribution.

[CalMacCQ]

I think its just a test thats needed. Would it help if I added the test case?

[RasmitDevkota]

Sorry for the late response - some help would definitely be appreciated! I've just been struggling to construct the pytket circuit correctly so that the circuit objects would be equal in the end.

[CalMacCQ]

Sorry for the late response - some help would definitely be appreciated! I've just been struggling to construct the pytket circuit correctly so that the circuit objects would be equal in the end.

No worries, I'm going on leave for a few days but I'll get back to you on Monday :)

[CalMacCQ]

Looking at this now :)

[cqc-melf]

@CalMacCQ what is the state of this? Do you need help?

[CalMacCQ]

I split off the fix that @RasmitDevkota made into a separate PR. #524 This change was merged and released some time ago.

I personally didn't have plans to work on this anymore. If someone can make a case that these are useful conversions to support and maintain then we can maybe come back to it.

[cqc-melf]

Please reopen and ping me if you think this is still relevant.