search

CQCL / pytket-qir

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

Prefer `init_reg` over `reg2var` for empty classical registers #57

Closed qartik closed 1 year ago

qartik commented 1 year ago

reg2var is too verbose for empty registers. I would like to propose init_reg instead which is not only concise but preserves valuable register size information.

I also added a Makefile and config files for pre-commit and ruff. While doing the latter, I ended up fixing some typos and improving some code.

I am happy to hear other suggestions such as perhaps using init_reg to completely replace reg2var by passing an initial value in the form of a decimal i64.

qartik commented 1 year ago

Have you discussed the changes with @peter-campora and if this will be supported by the backend in the same way?

Yes, once this new init_reg API is stabilized and accepted here, I will ensure the backend supports it as well.

I am not really sure if this is the right thing to do, I would assume that it might be better to keep the register size outside of qir. It is not really clear to me what the expected returns type are if we execute operations on registers with different sizes. Can you maybe give some more details on that?

The return types continue to be i64. Since the backend only consumes QIR, I believe that's the only place to store register size information, though I am open to alternatives if they exist. Think of the first argument to init_var as an additional metadata for the backend to distinguish it from real integers from other frontends such as Q# -> QIR. This extra information may also be used to decide do other optimizations at the backend (if needed).

We usually try to separate clean up / ci / infrastructure changes from new features. Do you think it would be possible to split this up into two PRs?

I can do that. But my logical changes are on top of the linting changes such as black pass, which seemed like was not run in quite some time. So if the linting changes look okay, I can create a PR for those first.

cqc-melf commented 1 year ago

We usually try to separate clean up / ci / infrastructure changes from new features. Do you think it would be possible to split this up into two PRs?

I can do that. But my logical changes are on top of the linting changes such as black pass, which seemed like was not run in quite some time. So if the linting changes look okay, I can create a PR for those first.

The current used pylint and black is run on every PR, for me it looks like that ruff is just following a different convention. I think it would be good to move that to a different PR. If there are benefits for changing the package we are using for that, we could think about changing that for our other pytket extensions as well. This should be updated in the readme as well that we are now using ruff, see https://github.com/CQCL/pytket-qir#formatting

I think it would be good to have an additional PR for changing the ci to use ruff and run ruff once on the code to have a clean commit on develop which only contains the changes related to that. (And doing the precommit, makefile, ... changes in an additional separate PR after that.)

I am happy to help you with that, splitting this up, might be a little bit more work.

qartik commented 1 year ago

The current used pylint and black is run on every PR, for me it looks like that ruff is just following a different convention. I think it would be good to move that to a different PR. If there are benefits for changing the package we are using for that, we could think about changing that for our other pytket extensions as well. This should be updated in the readme as well that we are now using ruff, see https://github.com/CQCL/pytket-qir#formatting

It's possible that since I used the most recent version of black, it did things differently. But it seems compatible with what was on the CI.

BTW, ruff doesn't format things. Most of the formatting was performed by black. But I will double check that when I create a separate PR.

cqc-melf commented 1 year ago

If the changes from you are done by just updating the black version we could do that as well and just add ruff in addition to that?

qartik commented 1 year ago

BTW, ruff doesn't format things. Most of the formatting was performed by black. But I will double check that when I create a separate PR.

Indeed, I was mistaken, a lot of changes came from my initial trials with ruff. But I was more careful in the separate PR for linting/automation improvements: https://github.com/CQCL/pytket-qir/pull/58