Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DumpMachine parsing in DumpUnitary #210

Merged
merged 14 commits into from Nov 22, 2019
Merged

Conversation

ironyman
Copy link
Contributor

@ironyman ironyman commented Oct 29, 2019

Untested... do we have tests for this?

#205

Copy link
Member

@tcNickolas tcNickolas left a comment

Choose a reason for hiding this comment

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

Thank you for helping us with this! I left one comment about parsing negative imaginary components of the amplitudes.

You make a valid point about the tests - this tool does not have them. It was written in great haste for our programming contest, and went pretty much unused since then (UnitaryPatterns kata is one of the more advanced ones, so not a lot of people make it as far as to solving it), so we've never invested in writing tests for it.

Would you be willing to help us improve the test coverage? It would be a slightly larger change, but not hugely so. There is no good way to validate the output written to DumpUnitary.txt, since each column might acquire an independent phase, and the resulting unitary might differ from the intended one. But the output written to DumpUnitaryPattern.txt will be unchanged, and having a test cover it will at least notify us of breaking changes like changes in output format of DumpMachine.

If you want to look into writing a test, a good test case in Q# code is the following:

operation TestUnitary (qs : Qubit[]) : Unit {
    S(qs[2]);
    S(qs[1]);
    H(qs[0]);
}

(and then let unitary = TestUnitary; in CallDumpUnitary) - this operation exercises amplitudes with both real and imaginary parts both positive and negative.

utilities/DumpUnitary/Driver.cs Outdated Show resolved Hide resolved
@ironyman
Copy link
Contributor Author

Not sure what kind test you had in mind. I added an expected value test using the gates you described.

@tcNickolas
Copy link
Member

Looks like there are some mismatches on .NET versions - we've updated to .NET Core 3.0 on Friday, so if you update the branch with the latest master, things should work with 3.0.
(Sorry I haven't had a chance to look at your PR again, got some unexpected travel to do, but I should have some time closer to the end of this week)

Copy link
Member

@tcNickolas tcNickolas left a comment

Choose a reason for hiding this comment

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

I took the liberty of changing .NET Core version in the new .csproj file to 3.0 - now the CI build passes, so we have the right combination of versions.

I left some comments - they are purely decorative, overall the changes and the new test look good.

Thank you! (and sorry for the delay reviewing this!)

utilities/DumpUnitary/Operations.qs Show resolved Hide resolved
utilities/DumpUnitary/Driver.cs Outdated Show resolved Hide resolved
utilities/DumpUnitary/Driver.cs Outdated Show resolved Hide resolved
utilities/DumpUnitary/Driver.cs Outdated Show resolved Hide resolved
utilities/DumpUnitary/Driver.cs Outdated Show resolved Hide resolved
utilities/DumpUnitaryTest/DumpUnitaryTest.csproj Outdated Show resolved Hide resolved
utilities/DumpUnitaryTest/UnitTest1.cs Outdated Show resolved Hide resolved
utilities/DumpUnitaryTest/UnitTest1.cs Outdated Show resolved Hide resolved
utilities/DumpUnitaryTest/UnitTest1.cs Outdated Show resolved Hide resolved
utilities/DumpUnitaryTest/UnitTest1.cs Outdated Show resolved Hide resolved
@tcNickolas
Copy link
Member

Please let me know if you'll continue working on this PR or if you'd like me to finish it - the remaining changes are cosmetic so it would be nice to do them and close this issue. Thank you!

@ironyman
Copy link
Contributor Author

Please let me know if you'll continue working on this PR or if you'd like me to finish it - the remaining changes are cosmetic so it would be nice to do them and close this issue. Thank you!

I'll try to work on it this weekend!

Copy link
Member

@tcNickolas tcNickolas left a comment

Choose a reason for hiding this comment

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

Looks good! I took the liberty to remove the unnecessary dump files - looks like they ended up in the commit accidentally - and to do a couple of whitespace changes - it seemed excessive to ask you to do them yourself.

I'll merge this once the CI build completes. Thank you!

@tcNickolas tcNickolas merged commit 0e878f0 into microsoft:master Nov 22, 2019
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.

None yet

2 participants