CQCL / pytket-qir

Public repo for the pytket-qir package
Apache License 2.0
6 stars 1 forks source link

speedup and cleanup #147

Closed cqc-melf closed 2 months ago

cqc-melf commented 2 months ago

Description

Speed up the conversion update pytket version add testcase for invalid register

Checklist

cqc-melf commented 2 months ago

If time taken to error is a concern, we could move some of the validity checking to earlier in the function (unless that will add a lot of overhead).

It was brought up on Tuesday that this is a concern, that is why I had moved the test to the beginning and made it optional. I made it optional because it takes quiet some time compared to other operations, for the circuit I looked at it generated additional ~5% / ~1min runtime. I am not sure how much this additional runtime is a concern. If this is not a problem I would just put it back in, if this is a problem, I am happy to update register calls and conversion to have additional checks for the incomplete registers to make sure they fails with a useful error message every time.

My personal opinion is that having the check at the beginning with an opt out option would be an acceptable compromise between all this considerations.

cqc-alec commented 2 months ago

If time taken to error is a concern, we could move some of the validity checking to earlier in the function (unless that will add a lot of overhead).

It was brought up on Tuesday that this is a concern, that is why I had moved the test to the beginning and made it optional. I made it optional because it takes quiet some time compared to other operations, for the circuit I looked at it generated additional ~5% / ~1min runtime. I am not sure how much this additional runtime is a concern. If this is not a problem I would just put it back in, if this is a problem, I am happy to update register calls and conversion to have additional checks for the incomplete registers to make sure they fails with a useful error message every time.

My personal opinion is that having the check at the beginning with an opt out option would be an acceptable compromise between all this considerations.

Another option would be to have another function like validate_circuit() that just performs these checks and which the user can choose to call before conversion.

I would be surprised if the time taken to report errors for huge invalid circuits were a major issue. But happy to go with whichever option you decide on.

cqc-melf commented 2 months ago

If time taken to error is a concern, we could move some of the validity checking to earlier in the function (unless that will add a lot of overhead).

It was brought up on Tuesday that this is a concern, that is why I had moved the test to the beginning and made it optional. I made it optional because it takes quiet some time compared to other operations, for the circuit I looked at it generated additional ~5% / ~1min runtime. I am not sure how much this additional runtime is a concern. If this is not a problem I would just put it back in, if this is a problem, I am happy to update register calls and conversion to have additional checks for the incomplete registers to make sure they fails with a useful error message every time. My personal opinion is that having the check at the beginning with an opt out option would be an acceptable compromise between all this considerations.

Another option would be to have another function like validate_circuit() that just performs these checks and which the user can choose to call before conversion.

I would be surprised if the time taken to report errors for huge invalid circuits were a major issue. But happy to go with whichever option you decide on.

I think having an additional function is also a good idea. I have added that, should be in 6a78ce7

I think this should address all the comment, if I have messed anything, please let me know.