Skip to content

Conversation

@RasmitDevkota
Copy link

Description

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

Related issues

Closes #514

@CalMacCQ
Copy link
Contributor

CalMacCQ commented Aug 26, 2025

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.

@CalMacCQ CalMacCQ changed the title implemented support for Var, Unary, and Binary operations during qisit_to_tk conversion implemented support for Var, Unary, and Binary operations during qiskit_to_tk conversion Aug 26, 2025
@RasmitDevkota
Copy link
Author

RasmitDevkota commented Aug 26, 2025

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
Copy link
Contributor

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

)

# Utility method to flatten conditions
def flatten_condition(_condition):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

@RasmitDevkota
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

CalMacCQ commented Sep 6, 2025

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

@CalMacCQ
Copy link
Contributor

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

@RasmitDevkota
Copy link
Author

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
Copy link
Contributor

CalMacCQ commented Sep 16, 2025

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
Copy link
Contributor

Looking at this now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect bits list referenced in _append_if_else_circuit

2 participants