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

_operator_map and _observable_map are now class attributes. #48

Merged
merged 2 commits into from
Sep 19, 2018

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Sep 18, 2018

Main changes:

  • _operator_map and _observable_map are now required class attributes for all devices. This simplifies plugin development, as the developer no longer needs to import DeviceError and raise issues if incorrect operators/observables are passed. Instead, this is handled in the background by Device.

  • Device.supported() now directly queries the operator and observable maps, and no longer needs to be overwritten by the developer either.

Note that _operator_map and _observable_map do not need to map to anything useful (i.e. they could all map to None, if the plugin developer wishes - it is entirely up to the developer). All that is required is that the supported keys are provided.

Other changes:

  • Added docstrings to missing methods, cleaned up openqml-sf plugin
  • Refactored default.qubit plugin to use the new suggested API
  • Minor bug fixes in openqml-pq to update it to the new API

…map are now required and are class attributes.
@josh146 josh146 added enhancement ✨ New feature or request devices 💻 Device or plugin API related labels Sep 18, 2018
Returns:
tuple(float, float): Mean photon number and variance.
"""
return state.mean_photon(wires), 0
Copy link
Member

@co9olguy co9olguy Sep 18, 2018

Choose a reason for hiding this comment

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

Do we want to return 0 for the variance here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I'm planning to submit a quick fix to SF today so that mean_photon also returns the variance

openqml/device.py Outdated Show resolved Hide resolved
openqml/device.py Outdated Show resolved Hide resolved
openqml/device.py Outdated Show resolved Hide resolved
# estimate the expectation value
# use central limit theorem, sample normal distribution once, only ok
# if shots is large (see https://en.wikipedia.org/wiki/Berry%E2%80%93Esseen_theorem)
ex = np.random.normal(ex, np.sqrt(var / self.shots))
Copy link
Member

Choose a reason for hiding this comment

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

Should we modify this so that it actually runs the simulation self.shots times?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm on the fence, what do you think? It would add significant overhead for large number of shots

Copy link
Member Author

Choose a reason for hiding this comment

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

Might be worth making an issue about this

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more that we need to be careful with a low number of shots; in that case, it is not a very good estimator. On the flip side, in that case, the overhead of repeatedly running the simulation wouldn't be too bad

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we use an heuristic to decide? I.e. for shots < 10 (arbitrarily chosen) we rerun the simulation, and for shots > 10, we use the central limit theorem

Copy link
Member

Choose a reason for hiding this comment

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

yeah, probably the best solution for now. Maybe a number higher than 10 though?

Copy link
Member

@co9olguy co9olguy left a comment

Choose a reason for hiding this comment

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

On the whole, looks good for merging. Most of my comments are small points/questions

@cgogolin
Copy link
Contributor

cgogolin commented Sep 18, 2018 via email

@josh146 josh146 merged commit 0838419 into master Sep 19, 2018
@josh146 josh146 deleted the op_map_class_attr branch September 19, 2018 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devices 💻 Device or plugin API related enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants