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

[IMPROVEMENT] Replace DeviceType with EmulatorType #72

Merged
merged 4 commits into from
Apr 18, 2023
Merged

Conversation

Augustinio
Copy link
Collaborator

@Augustinio Augustinio commented Apr 6, 2023

@anneclairelh
Copy link
Contributor

I am sure there is a good reason but I'm not sure to see it, why are we doing this change ?

@Augustinio
Copy link
Collaborator Author

@anneclairelh for some reason I can't reply directly to your comment.

Basically the QPU DeviceType was never really used. It was just a flag to indicate that we weren't using emulators. Since QPU device types are extracted from the sequence.

In public-apis we're removing QPU as a device type since it's not an actual device type and lead to confusion regarding what it meant. Similarly it's a way to make the code clearer and avoid users to potentially get confused with the meaning of this QPU device type.

@anneclairelh
Copy link
Contributor

@anneclairelh for some reason I can't reply directly to your comment.

Basically the QPU DeviceType was never really used. It was just a flag to indicate that we weren't using emulators. Since QPU device types are extracted from the sequence.

In public-apis we're removing QPU as a device type since it's not an actual device type and lead to confusion regarding what it meant. Similarly it's a way to make the code clearer and avoid users to potentially get confused with the meaning of this QPU device type.

Of course, but my question is, do we know that we won't have other QPU device types ? Is the intention behind this that we don't want to expose device types like "FRESNEL" in a public repo ?

@anneclairelh
Copy link
Contributor

I am sure there is a good reason but I'm not sure to see it, why are we doing this change ?

Indeed Github is pretty bad and only allows you to quote ;)

@MatthieuMoreau0
Copy link
Collaborator

@anneclairelh

Of course, but my question is, do we know that we won't have other QPU device types ? Is the intention behind this that we don't want to expose device types like "FRESNEL" in a public repo ?

We will have other QPU device type, we just don't want to hard code it inside the SDK, as these can be fetched directly from the cloud device specs anyway (and we don't want to have to update the package everytime we add a QPU to the list). Moreover to select a precise QPU, the user will select it directly inside their pulser sequence.

Having an extra argument on the SDK was making things confusing, the user would select device_type=QPU but the resulting batch would have its attribute device_type set to something like IROISEMVP

Copy link
Collaborator

@MatthieuMoreau0 MatthieuMoreau0 left a comment

Choose a reason for hiding this comment

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

Nice job LGTM, one tiny suggestion

README.md Outdated
Comment on lines 176 to 177
from sdk.device.emulator_types import EmulatorType
from sdk.device.configuration import EmuFreeConfig
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: for convenience for the users, could we export these objects in the sdk.device module ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@MatthieuMoreau0 MatthieuMoreau0 left a comment

Choose a reason for hiding this comment

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

This introduces breaking changes can you bump the minor instead of the patch please to follow semantic versioning ? See #73

@Augustinio
Copy link
Collaborator Author

This introduces breaking changes can you bump the minor instead of the patch please to follow semantic versioning ? See #73

new version is 0.2.0

@Augustinio Augustinio merged commit da2da8e into dev Apr 18, 2023
@Augustinio Augustinio deleted the alg/remove-qpu branch April 18, 2023 16:43
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.

3 participants