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 connection to device spec API #429

Merged
merged 27 commits into from Jul 8, 2020
Merged

Conversation

thisac
Copy link
Contributor

@thisac thisac commented Jun 24, 2020

Context:
A new device spec API has been created and thus needs to be connected to Strawberry Fields.

Description of the Change:
A new DeviceSpec class has been added, containing device specifications for a certain target, along with a connection.get_device method that fetches the device specifications for the target chip and returns a DeviceSpec object.

Benefits:
Information about the hardware devices can now be refreshed and used in Strawberry Fields without the need to update Strawberry Fields.

Possible Drawbacks:
N/A

Related GitHub Issues:
N/A

@thisac thisac changed the title Add connection to device spec API [WIP] Add connection to device spec API Jun 24, 2020
@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #429 into master will increase coverage by 0.09%.
The diff coverage is 99.35%.

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
+ Coverage   97.84%   97.94%   +0.09%     
==========================================
  Files          58       69      +11     
  Lines        6784     7096     +312     
==========================================
+ Hits         6638     6950     +312     
  Misses        146      146              
Impacted Files Coverage Δ
strawberryfields/apps/plot.py 100.00% <ø> (ø)
strawberryfields/apps/train/embed.py 100.00% <ø> (ø)
...awberryfields/backends/gaussianbackend/__init__.py 100.00% <ø> (ø)
strawberryfields/backends/gaussianbackend/ops.py 100.00% <ø> (+2.70%) ⬆️
strawberryfields/circuitspecs/circuit_specs.py 100.00% <ø> (ø)
strawberryfields/circuitspecs/gaussian_unitary.py 100.00% <ø> (ø)
strawberryfields/circuitspecs/tensorflow.py 100.00% <ø> (ø)
strawberryfields/backends/fockbackend/ops.py 94.04% <94.11%> (+2.04%) ⬆️
strawberryfields/ops.py 98.84% <96.29%> (-0.19%) ⬇️
strawberryfields/backends/tfbackend/ops.py 96.69% <97.88%> (-0.17%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f77e879...47680f6. Read the comment docs.

.pylintrc Outdated
@@ -10,7 +10,7 @@ extension-pkg-whitelist=numpy,tensorflow,scipy,networkx,strawberryfields
# (useful for modules/projects where namespaces are manipulated during runtime
# and thus existing member attributes cannot be deduced by static analysis. It
# supports qualified module names, as well as Unix pattern matching.
ignored-modules=numpy,tensorflow,scipy,networkx,strawberryfields,strawberryfields.parameters
ignored-modules=numpy,tensorflow,scipy,networkx,strawberryfields,strawberryfields.parameters,collections.abc
Copy link
Member

Choose a reason for hiding this comment

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

For some reason, pylint/codefactor was saying 'No Sequence in module collections.abc'

@josh146 josh146 changed the title [WIP] Add connection to device spec API Add connection to device spec API Jun 26, 2020
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

This looks great on my end @thisac! Let's get @antalszava's thoughts before we merge it in

@josh146 josh146 requested a review from antalszava June 26, 2020 05:36
@thisac thisac marked this pull request as ready for review June 26, 2020 15:17
Comment on lines +387 to +391
def __eq__(self, other):
if len(self.ranges) != len(other.ranges):
return False

return all(i == j for i, j in zip(self.ranges, other.ranges))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note here: this assumes that the Ranges obtained contain Range objects in a monotonously increasing fashion. I'd think that that's okay, as we'd obtained these objects through the API. Perhaps a check could be placed in __init__ for it? 🤔 (though out of scope I think)

from strawberryfields.circuitspecs import Ranges

x = Ranges([0], [0.2, 0.55], [1.0])
y = Ranges([0], [1.0], [0.2, 0.55])

assert x == y
AssertionError                            Traceback (most recent call last)
<ipython-input-6-cdb229d2c821> in <module>
      4 y = Ranges([0], [1.0], [0.2, 0.55])
      5 
----> 6 assert x == y

AssertionError: 

Copy link
Member

Choose a reason for hiding this comment

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

Good point @antal!

.github/CHANGELOG.md Outdated Show resolved Hide resolved
**Example**

>>> spec.gate_parameters
{'squeezing_amplitude_0': x=0, x=1, 'phase_0': x=0, 0≤x≤6.283185307179586}
Copy link
Contributor

Choose a reason for hiding this comment

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

(Out of scope): find 'squeezing_amplitude_0': x=0, x=1 slightly confusing, would perhaps prefer 'squeezing_amplitude_0': x ∈ {0, 1}. For intervals this would get slightly more complicated, having to have unions of sets with U. Could just be me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! We're thinking about updating the Ranges class to produce nicer/clearer outputs (it's quite messy right now since the actual output would be {'squeezing_amplitude_0': squeezing_amplitude_0=0, squeezing_amplitude_0=1, 'phase_0': phase_0=0, 0≤phase_0≤6.283185307179586} in this case. 😕

This would be in a separate PR, though, and it could also solve the issue you mentioned above with the ordering.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, let's save this for a subsequent PR

Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Left a couple of minor suggestions, but overall looks good to me! 💯

@josh146
Copy link
Member

josh146 commented Jul 7, 2020

@thisac, @antalszava: I've gone through and made suggested changes, and merged in master. Might be good if you have one last look through this, and then we can merge it into master and work on finishing #432 🙂

@antalszava
Copy link
Contributor

💯

@thisac
Copy link
Contributor Author

thisac commented Jul 7, 2020

Thanks @josh146. Looks good! I only had one, very minor, comment on the way Ranges is called for the gate_parameters property. Other than that, I think it looks fine. 🙂

josh146 and others added 2 commits July 8, 2020 09:59
Co-authored-by: Theodor <theodor@xanadu.ai>
@josh146 josh146 merged commit e980b02 into master Jul 8, 2020
@josh146 josh146 deleted the add_device_API_connection branch July 8, 2020 07:00
nquesada added a commit that referenced this pull request Sep 1, 2020
* Add connection to device spec API (#429)

* Implement a get_device method

* Return DeviceSpec fro get_device

* Split get_device method

* Add devicespec class

* Fix formatting

* write out docstrings

* add to documentation

* linting

* update changelog

* add PR link

* done

* blacking

* undo weird string concatenation

* Fix minor error

* Add tests

* Fix failing test

* adding tests

* add initialization test

* Add kwargs and return to docstring

* Update .github/CHANGELOG.md

Co-authored-by: antalszava <antalszava@gmail.com>

* suggested changes

* Update strawberryfields/api/devicespec.py

Co-authored-by: Theodor <theodor@xanadu.ai>

* suggested changes

Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>

* Update analytic expression in variance test (#436)

* Update analytic expression

* Decrease amount of displacement & squeezing to remove custom tolerance

* Applying suggestion to simplify squared_term

* Raise error for complex arguments to Coherent, DisplacedSqueezed and Dgate gates (#428)

* error/warning if Dgate, Sgate, S2gate are misused

* added any() in complex test to handle batching

* fixed theta --> r

* simpler conditional

* fixed usage of any()

* added collections.abc to ignored modules

* added warn+error for Coherent, DisplacedSqueezed

* Use polar in remaining tests

* Remove unnecessary complex var

* Removing warnings; removing errors from BSgate and S2gate

* Formatting

* Modifying further tests

* Updating test of warning for tests on errors

* Adjust test docstring

* Creating test class

* Test

* Move parametrize

Co-authored-by: antalszava <antalszava@gmail.com>

* Add duschinsky function to qchem utils (#434)

* Device spec API implementation (#432)

* Implement a get_device method

* Return DeviceSpec fro get_device

* Split get_device method

* Add devicespec class

* Fix formatting

* write out docstrings

* add to documentation

* linting

* update changelog

* add PR link

* done

* blacking

* undo weird string concatenation

* Fix minor error

* Add tests

* Fix failing test

* adding tests

* add initialization test

* Add kwargs and return to docstring

* Update .github/CHANGELOG.md

Co-authored-by: antalszava <antalszava@gmail.com>

* Change name of circuitspecs to compiler

* Add spec method to engine

* Update program.compile to support DeviceSpec

* Add default_compiler method

* Update circuitspecs name to compiler in tests

* finish compiler renaming

* blacking

* fix tests

* fix tests

* fix documentation

* fix documentation

* fix documentation

* linting

* black

* Minor fixes

* Update docstring

* Add tests

* suggested changes

* black

* fix failing tests

* merge master

* linting

* increase coverage

* linting

* added logging

* tidying up

* fix test

* fix test

* linting

* Update strawberryfields/compilers/xunitary.py

Co-authored-by: antalszava <antalszava@gmail.com>

* Update signature of `Program.compile`

* Update code and docs to use new compile signature

* Update tests to use the new `Program.compile` signature

* Remove unused import

* Apply suggestions from code review

Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>

* First working version. Mostly copies and pastes the outcome of the ADR

* Adds docstrings

* Adds docstrings

* Implements reshaper for the samples

* First test

* Correct implementation of reshape

* Fixes tests

* Not quite yet working test for EPR generation

* Update test_tdmprogram.py

* Implements minor change in engine.py

* Adds tests for most errors

* minor lint fix

* minor lint fix

* removes matplotlib import

* fix import

* add default

* tests for ghz and cluster states + correction of BS operation

Found a convention error: It seems important that the qumodes of a BSgate are ordered with the "older" qumode (lower index) mentioned first, so

ops.BSgate(p[0]) | (q[1], q[2])

instead of

ops.BSgate(p[0]) | (q[2], q[1])

* test for tokyo's million-mode cluster state

* Minor changes

* 2D cluster state from DTU implemented

* some refactoring and a bunch of comments

* adding tdmprogram2 supporting individual conveyor belts for each spatial mode

* local changes in examples

* Renames tdmprograms

* Fix reshape_samples

* Removes incorrectly committed files

* changing "tdmprogram2" back to "tdmprogram" in the test script

* Fix reshape_samples

* adaption to new samples shape

* Add argument to reshape_samples

* Fix reshape_samples

* small correction

* added test for tokyo's 2D state

* Fix reshaping

* Add docstring to reshape_samples

* Tokyo2D running successfully

Co-authored-by: Theodor <theodor@xanadu.ai>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: ziofil <ziofil@users.noreply.github.com>
Co-authored-by: soranjh <40344468+soranjh@users.noreply.github.com>
Co-authored-by: fab1an-q <62264296+fab1an-q@users.noreply.github.com>
Co-authored-by: Theodor Isacsson <theodor.isacsson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants