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

Add unit tests for encoding module #274

Open
wants to merge 8 commits into
base: unitary-hack
Choose a base branch
from

Conversation

SaashaJoshi
Copy link

@SaashaJoshi SaashaJoshi commented Jun 3, 2024

Adding unit tests for

  • General encoder
  • Phase encoder
  • Multi-phase encoder
  • State encoder
    - [ ] Magnitude encoder

Addresses #261

Copy link
Collaborator

Choose a reason for hiding this comment

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

lgtm!

@01110011011101010110010001101111 01110011011101010110010001101111 linked an issue Jun 5, 2024 that may be closed by this pull request
@01110011011101010110010001101111 01110011011101010110010001101111 changed the base branch from main to unitary-hack June 5, 2024 17:02
Copy link
Collaborator

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

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?

Copy link
Author

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):

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!

Copy link
Author

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!

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)

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.

@01110011011101010110010001101111
Copy link
Collaborator

@SaashaJoshi Could you update this by the end of today to complete the bounty? Feel free to ask any questions if needed!

@SaashaJoshi
Copy link
Author

I'll try my best to finish it today. I am on a little time crunch.

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.

Develop the TorchQuantum Testing Suite
2 participants