Closed troelsfr closed 2 years ago
Sorry for the delay in getting to this review. The new transformation passes look good for the most part, I just had a few clarifying questions about intent. However, the Q# files under TargetTestScripts don't make sense to me. Either I'm missing the intent here or all of those files can be removed as they are not testing anything meaningful (and in some cases refer to internal operations that cannot be called directly). I don't think the QAT repo is not the right place to perform any testing of target packages.
The Q# tests being ran produces the QIR implementation of the respective operations. The task was to create tests to check if these would validate against a specific target using QAT. Whether the QAT repo is the right place for these to live, I will leave as a discussion between you and @bettinaheim. My view is that is fine for them to live in the QAT repo, but it is not something I feel strongly about.
The Q# tests being ran produces the QIR implementation of the respective operations. The task was to create tests to check if these would validate against a specific target using QAT. Whether the QAT repo is the right place for these to live, I will leave as a discussion between you and @bettinaheim. My view is that is fine for them to live in the QAT repo, but it is not something I feel strongly about.
In this case, I think we need to consider which cross-repo dependencies we introduce and how we imagine maintaining them going forward. In addition, the test functions as currently written won't actually validate any of the complexity in the targeting logic of Q#. They'll need to test the callable specializations like Controlled
and Controlled Adjoint
with different numbers of control qubits to actually trigger anything more that very simple decompositions. There is definitely a spread of tests we should write, but they are specific enough to implementation details of how Q# target packages are authored that I think they should live next to those packages and take a dependency on QAT rather than the other way around.
I think the remainder of the PR is reasonable to merge, but the targeting tests for Q# are something that would at the very least need to be moved out to a separate PR to be considered on their own.
@swernli Agreed on the need to validate Adjoint and Controlled versions. Regarding location, I don't believe the location is exclusive; tests here validate that qat processes things as it should taking a fixed version dependency on the Q# packages. Any changes to qat have a risk to break the processing of code, and the implementations in the target packages is reasonable to validate. I also agree that the tests here don't preclude that a bad change is made in the target packages, and the target packages hence need to have their own tests to ensure that when they are changed they remain valid. What tests the target packages have or do not have is irrelevant, however, for what tests live here.
This PR introduces many smaller changes that allows us to test target operations. These changes include: