-
Notifications
You must be signed in to change notification settings - Fork 200
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
Add unit tests for encoding
module
#274
base: unitary-hack
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Discussed with the team and added a few more comments.
Essentially, we’re looking for more tests that check the functionality, so comparing the encoders against either hardcoded expected results or against an existing well-tested function from another library. It would be great if you could look into that for the encoders (as I believe you did for the StateEncoder).
Thanks so much for your hard work!
qdev = QuantumDevice(2) | ||
with mock.patch.object(encoder, "func") as mock_func: | ||
encoder(qdev, torch.rand(2, 4)) | ||
assert mock_func.call_count >= 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m unsure if this is necessary; what exactly are you checking here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am mocking the func
input here. Since this is a unit test for encoders, I need to isolate the functionality of the encoder regardless of the behaviour displayed by func
.
func
under the hood is a unitary gate. Unit testing for any gate input as func
should be done in a separate file, just like how tests/operators/Controlled_Unitary
is tested.
So you test the func
separately + test the encoder
separately = and then this indicates that they together work as you want.
"batch_size, wires, funcs", | ||
[(2, 5, ["ry", "phaseshift"]), (1, 4, ["u2"]), (3, 1, ["u3"])], | ||
) | ||
def test_phase_encoding(self, batch_size, wires, funcs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the StateEncoder, if you could develop the tests to verify the answer is correct (so compare the encoding to some baselines) for each of the encoders. These can either be hardcoded expected answers or compare them to the results of some library.
If you can do this for all of the encoders, that would be amazing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will definitely look into it.
PS: Are you thinking along the lines of an integration test? I have never written one but I will try!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just treat it like writing some unit tests; if you need a reference, you can check out the other tests.
""" | ||
""" | ||
# Validate inputs | ||
self.validate_inputs(qdev, x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to check if we want this @Hanrui-Wang. The pro is that it’s safer from errors but also could result in some extra overhead.
@SaashaJoshi Could you update this by the end of today to complete the bounty? Feel free to ask any questions if needed! |
I'll try my best to finish it today. I am on a little time crunch. |
Adding unit tests for
- [ ] Magnitude encoderAddresses #261