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.

Sign up for GitHub

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

Jump to bottom

Fix run_options not being used #500

Merged
merged 4 commits into from
Dec 8, 2020
Merged

Fix run_options not being used #500

merged 4 commits into from
Dec 8, 2020

Conversation

thisac
Copy link
Contributor

@thisac thisac commented Dec 4, 2020

Context:
Setting the number of shots in TDMProgram.run_options doesn't change the number of shots used when calling eng.run(), without passing a shots value.

Description of the Change:
Shots now checked and retrieved in the following order: shots passed via eng.run(prog, shots=shots) > prog.run_options > default value, which is 1. The left-most available being the one that is used.

Benefits:
If TDMProgram.run_options is set, that value is used rather than falling back on the default value. This can be the case, for example, when converting from a Blackbird script. Without this fix the shots values set in the Blackbird script would simply be ignored when running the engine.

Possible Drawbacks:
None

Related GitHub Issues:
None

@thisac thisac self-assigned this Dec 4, 2020
@codecov
Copy link

codecov bot commented Dec 4, 2020 •
edited
Loading

Codecov Report

Merging #500 (09d2b37) into master (75e3c26) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #500   +/-   ##
=======================================
  Coverage   97.85%   97.85%           
=======================================
  Files          70       70           
  Lines        7313     7313           
=======================================
  Hits         7156     7156           
  Misses        157      157
Impacted Files Coverage Δ
strawberryfields/engine.py 96.13% <100.00%> (ø)

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 75e3c26...09d2b37. Read the comment docs.

josh146
josh146 approved these changes Dec 7, 2020
View reviewed changes
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.

Nice 💪

strawberryfields/engine.py
@@ -424,8 +424,9 @@ def run(self, program, *, args=None, compile_options=None, **kwargs):
not isinstance(program, collections.abc.Sequence) and program.type == "tdm"
)
if valid_tdm_program:
# priority order for the shots value should be kwargs > run_options > 1
shots = kwargs.get("shots", program.run_options.get("shots", 1))
Copy link
Member

Choose a reason for hiding this comment

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

I like how this use of nested get makes it easy to see the priority

antalszava
antalszava reviewed Dec 7, 2020
View reviewed changes
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.

Looking good! 💯 Maybe a test case could be helpful to cover the previous non-working case and verify that shots are extracted well now.

antalszava
antalszava reviewed Dec 7, 2020
View reviewed changes
tests/frontend/test_engine.py
prog.run_options = {"shots": 5}
results = eng.run(prog, shots=2)
assert results.samples.shape[0] == 2
assert prog.run_options["shots"] == 5
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests look awesome! 🥇 🚀

antalszava
antalszava reviewed Dec 7, 2020
View reviewed changes
tests/frontend/test_tdmprogram.py
prog.run_options = {"shots": 5}
results = eng.run(prog, shots=2)
assert results.samples.shape[0] == 2
assert prog.run_options["shots"] == 5
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

antalszava
antalszava approved these changes Dec 7, 2020
View reviewed changes
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.

Looks good to me! 💯

@josh146 josh146 merged commit b9d9b99 into master Dec 8, 2020
@josh146 josh146 deleted the run-options-tdm branch December 8, 2020 08:24
nquesada added a commit that referenced this pull request Jan 19, 2021
@nquesada @thisac
@josh146 @antalszava @brownj85 @trbromley @corvust @jswinarton
Gkps merge master (#528)
4b9d828 
* Fix issue with single parameter list (#503)

* Fix issue with single parameter list

* Fix pyliny import complaint

* Update changelog

* removes unused variables in test_tdmprogram

* Fix run_options not being used (#500)

* Fix run_options not being used

* Update changelog

* Add tests

* removes unused variables in test_tdmprogram (#504)

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

* Add program code generator function (#496)

* Add serialize function

* Run black

* Remove f

* Updates from code review

* Improve operations handling

* Remove duplicate line

* Fix generate_code + add tests

* Tidy things up a bit

* Add forgotten factor

* Run black

* Update changelog

* Fixes from code review

* Update strawberryfields/io.py

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

* change argument name

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

* Add wigner plotting (#495)

* Add wigner plotting to program

* Move plotting to plot module

* Remove numpy import

* Fixes after code-review

* Tidy up wigner plotting

* Run black

* Add test

* Update colours

* Update changelog

* rename tests

* Update strawberryfields/plot.py

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

* add contours arg

* Update tests

* Update changelog

* Apply suggestions from code review

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

* Remove url

* fix string

* fix arg name

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

* Adds api_version property, adds 'Accept-Version' to headers (#512)

* Fock state and quadrature plotting  (#510)

* Fock state plotting draft

* Updates, error, test

* test & imports

* Test chart generation

* Formatting

* Remove unnecessary comment

* Two-mode test

* cutoff in test adjust

* quad and adjust fock prev chart

* Formatting

* Adjust

* quad test

* generate_quad_chart test

* Update tests/frontend/test_sf_plot.py

* update

* data always has two elements

* reset Makefile

* changelog, adjust chart title

* ket latex render

* latex in fock plot title

* adjust tests for latex

* Update strawberryfields/plot.py

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

* Update strawberryfields/plot.py

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

* adjust

* Update strawberryfields/plot.py

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

* marginal

* Update strawberryfields/plot.py

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

* Update strawberryfields/plot.py

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

* docstring

* adjust tests for Fock

* quad tests

* docstring

* Move obtaining the Wigner function of the state to generate_wigner_chart

* Docstring

* add plot page

* Update strawberryfields/plot.py

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

* base state render

* docstring basestate render change

Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Theodor <theodor@xanadu.ai>

* Fix bug in Dgate, Coherent, and DisplacedSqueezed (#507)

* Support TF tensors in batch form

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

* Add test

* Add to changelog

* Update PR in changelog

* New line

* Extend to other gates

* Update changelog

* Update

* Update test

* Update .github/CHANGELOG.md

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

* Fix typo

* Fix

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

* Update tdm docstring (#519)

* removes unused variables in test_tdmprogram

* updates docstring

* Implement OAuth refresh token flow (#520)

* Implement OAuth refresh token flow

* Apply suggestions from code review

Formatting changes

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

* Add correct token refresh path

* update path, update getting the access token, remove url wrapping in _request

* Formatting with black

* move dict init into func

* Updates

* refresh access token unit tests

* no print

* User request.post directly, update tests

* Wrapped request test, updates

* Updates

* Formatting

* Remove access token refreshing from init

* Update tests/api/test_connection.py

* Update tests/api/test_connection.py

* Update tests/api/test_connection.py

* Update tests/api/test_connection.py

* Update strawberryfields/api/connection.py

Co-authored-by: Jeremy Swinarton <jeremy@swinarton.com>

* update msg in test

* changelog

Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: Antal Szava <antalszava@example.com>
Co-authored-by: Jeremy Swinarton <jeremy@swinarton.com>

* Increment version number to v0.17.0 (#523)

* bump version and update changelog

* update about

* remove old current release

* minor tweaks

* dev bump (#524)

* Update photonic_hardware.rst (#526)

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: Jack Brown <jack@xanadu.ai>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tim Leisti <corvust@users.noreply.github.com>
Co-authored-by: Antal Szava <antalszava@example.com>
Co-authored-by: Jeremy Swinarton <jeremy@swinarton.com>
nquesada added a commit that referenced this pull request Jan 20, 2021
@nquesada @thisac
@josh146 @antalszava @brownj85 @trbromley @corvust @jswinarton
dummy change (#530)
adfd1d3 
* Fix issue with single parameter list (#503)

* Fix issue with single parameter list

* Fix pyliny import complaint

* Update changelog

* Fix run_options not being used (#500)

* Fix run_options not being used

* Update changelog

* Add tests

* removes unused variables in test_tdmprogram (#504)

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

* Add program code generator function (#496)

* Add serialize function

* Run black

* Remove f

* Updates from code review

* Improve operations handling

* Remove duplicate line

* Fix generate_code + add tests

* Tidy things up a bit

* Add forgotten factor

* Run black

* Update changelog

* Fixes from code review

* Update strawberryfields/io.py

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

* change argument name

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

* Add wigner plotting (#495)

* Add wigner plotting to program

* Move plotting to plot module

* Remove numpy import

* Fixes after code-review

* Tidy up wigner plotting

* Run black

* Add test

* Update colours

* Update changelog

* rename tests

* Update strawberryfields/plot.py

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

* add contours arg

* Update tests

* Update changelog

* Apply suggestions from code review

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

* Remove url

* fix string

* fix arg name

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

* Adds api_version property, adds 'Accept-Version' to headers (#512)

* Fock state and quadrature plotting  (#510)

* Fock state plotting draft

* Updates, error, test

* test & imports

* Test chart generation

* Formatting

* Remove unnecessary comment

* Two-mode test

* cutoff in test adjust

* quad and adjust fock prev chart

* Formatting

* Adjust

* quad test

* generate_quad_chart test

* Update tests/frontend/test_sf_plot.py

* update

* data always has two elements

* reset Makefile

* changelog, adjust chart title

* ket latex render

* latex in fock plot title

* adjust tests for latex

* Update strawberryfields/plot.py

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

* Update strawberryfields/plot.py

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

* adjust

* Update strawberryfields/plot.py

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

* marginal

* Update strawberryfields/plot.py

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

* Update strawberryfields/plot.py

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

* docstring

* adjust tests for Fock

* quad tests

* docstring

* Move obtaining the Wigner function of the state to generate_wigner_chart

* Docstring

* add plot page

* Update strawberryfields/plot.py

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

* base state render

* docstring basestate render change

Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Theodor <theodor@xanadu.ai>

* Fix bug in Dgate, Coherent, and DisplacedSqueezed (#507)

* Support TF tensors in batch form

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

* Add test

* Add to changelog

* Update PR in changelog

* New line

* Extend to other gates

* Update changelog

* Update

* Update test

* Update .github/CHANGELOG.md

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

* Fix typo

* Fix

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

* Update tdm docstring (#519)

* removes unused variables in test_tdmprogram

* updates docstring

* Implement OAuth refresh token flow (#520)

* Implement OAuth refresh token flow

* Apply suggestions from code review

Formatting changes

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

* Add correct token refresh path

* update path, update getting the access token, remove url wrapping in _request

* Formatting with black

* move dict init into func

* Updates

* refresh access token unit tests

* no print

* User request.post directly, update tests

* Wrapped request test, updates

* Updates

* Formatting

* Remove access token refreshing from init

* Update tests/api/test_connection.py

* Update tests/api/test_connection.py

* Update tests/api/test_connection.py

* Update tests/api/test_connection.py

* Update strawberryfields/api/connection.py

Co-authored-by: Jeremy Swinarton <jeremy@swinarton.com>

* update msg in test

* changelog

Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: Antal Szava <antalszava@example.com>
Co-authored-by: Jeremy Swinarton <jeremy@swinarton.com>

* Increment version number to v0.17.0 (#523)

* bump version and update changelog

* update about

* remove old current release

* minor tweaks

* dev bump (#524)

* Update photonic_hardware.rst (#526)

* dummy change

* Update strawberryfields/backends/bosonicbackend/backend.py

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: Jack Brown <jack@xanadu.ai>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tim Leisti <corvust@users.noreply.github.com>
Co-authored-by: Antal Szava <antalszava@example.com>
Co-authored-by: Jeremy Swinarton <jeremy@swinarton.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antalszava antalszava antalszava approved these changes

@josh146 josh146 josh146 approved these changes

Assignees

@thisac thisac

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thisac @josh146 @antalszava