Merge the demo branch into master #319
Conversation
…variable test; adding teardown logic for env vars; correcting typo
… DEFAULT_CONFIG as module attribute
| tensorflow-tensorboard>=0.1.8 | ||
| tensorflow==1.3 |
There was a problem hiding this comment.
I feel really confused now 😅
There was a problem hiding this comment.
This needs to be changed, good eye!
Edit: oh, it needs to be changed after the tf2 branch is merged in, not yet.
There was a problem hiding this comment.
Didn't find any major things that would need changing here 😊 I was not too sure if we wanted to remove the TF requirements for now. Apart from that I've left some further comments/suggestions. One thing for formatting would be to adjust the date of the licenses, though that would be global for SF (not just limited to demo).
Co-Authored-By: antalszava <[email protected]>
| Functions | ||
| --------- |
There was a problem hiding this comment.
I don't know why, but I get a WARNING: Title level inconsistent for this part 🤔
There was a problem hiding this comment.
Also noticed a few other errors upon making the docs, e.g., WARNING: autodoc: failed to import class 'Chip0Specs' - are these ok?
There was a problem hiding this comment.
Weird 🤔 Works fine for me
| >>> save("test_compiled.xbb", prog) | ||
|
|
||
|
|
||
| Tips and tricks |
There was a problem hiding this comment.
Could rename this to "Advanced", do we expect the average user to know the signal and idler modes?
| :tooltip: Photonic gate visualization | ||
| :description: :doc:`/gallery/gate_visualisation/GateVisualisation` | ||
| :tooltip: RemoteEngine | ||
| :description: :doc:`/introduction/remote` | ||
| :figure: /gallery/gate_visualisation/GateVisualisation.gif |
There was a problem hiding this comment.
Any ideas for a nice thumbnail? 🤔 We need the iPad now!
| <div style='clear:both'></div> | ||
| <br> | ||
|
|
||
| Hardware |
There was a problem hiding this comment.
Would potentially recommend putting this part above or directly below applications
Co-Authored-By: Tom Bromley <[email protected]>
|
@trbromley, thanks! I agree with your comments re: the configuration page and the hardware tutorial. However, I think it's best to address them in separate PRs post-demo merge. Partly because this PR is so big, and blocking a few other changes required for release. |
|
|
||
| >>> sf.store_account("MYAUTH") | ||
|
|
||
| to create a configuration file containing your credentials. This is the recommended approach. |
There was a problem hiding this comment.
Good points @trbromley, agreed
|
|
||
| .. code-block:: bash | ||
|
|
||
| $ sf configure --token MYAUTH |
There was a problem hiding this comment.
Might not be clear to users what MYAUTH is
.github/CHANGELOG.md
Outdated
|
|
||
| * Adds the `x_quad_values` and `p_quad_values` methods to the `state` class. | ||
| This allows calculation of x and p quadrature | ||
| probability distributions by integrating across the Wigner function. | ||
| [#270](https:/XanaduAI/strawberryfields/pull/270) | ||
| [(#270)](https:/XanaduAI/strawberryfields/pull/270) | ||
|
|
||
| * Adds support in the applications layer for node-weighted graphs. Users can sample from graphs | ||
| with node weights using the WAW encoding and input node weights into search algorithms in the |
There was a problem hiding this comment.
Users might not know what the WAW encoding is
There was a problem hiding this comment.
I suggest we address this is a new smaller PR after demo is merged, as it is a pre-existing changelog entry.
There was a problem hiding this comment.
🤔 Currently the user could look at the linked PRs to find out
To include it here, are we able to have math equations in markdown?
(but yes I agree this could be done outside of this PR)
There was a problem hiding this comment.
Easiest thing is to not give any specifics that we have to define. Maybe "sample from graphs with node weights using special-purpose encodings"
| [#311](https:/XanaduAI/strawberryfields/pull/311) | ||
|
|
||
| ### Bug fixes | ||
| <h3>Bug fixes</h3> | ||
|
|
||
| * Symbolic Operation parameters are now compatible with TensorFlow 2.0 objects. |
There was a problem hiding this comment.
But why would a user want to use a tensorflow 2.0 object? 🤔
| [#280](https:/XanaduAI/strawberryfields/pull/280) | ||
|
|
||
| [(#280)](https:/XanaduAI/strawberryfields/pull/280) | ||
|
|
||
| * Removed two unnecessary else statements that pylint complained about. |
There was a problem hiding this comment.
| * Removed two unnecessary else statements that pylint complained about. | |
| * Removed two unnecessary else statements. |
imo, not even worth putting in changelog
|
|
||
| >>> results = eng.run(prog, shots=20) | ||
| Job e6ead866-04c9-4d48-ba28-680e8639fc41 is sent to server. | ||
| >>> results.samples.T |
There was a problem hiding this comment.
I don't think so. It should return all samples as (shots, modes) now, so the transposing of results.samples shouldn't be necessary.
Co-Authored-By: Nathan Killoran <[email protected]> Co-Authored-By: Theodor <[email protected]>
This is addressed in #321 - should be a few minutes to review, but wanted to do as a PR to keep the change clear. |
* Add apps example * Clarify WAW encoding
| eng = RemoteEngine(program.target) | ||
|
|
||
| sys.stdout.write("Executing program on remote hardware...\n") | ||
| result = eng.run(program) |
There was a problem hiding this comment.
Ah, I noticed something with the RemoteEngine implementation that may not be ideal... The run() method accepts a shots argument, which overrides the value specified in the Blackbird script (which it probably shouldn't), and defaults to 1 if not given. So this line here would run every program with 1 shot regardless of what is in the script, which is probably not the intended behaviour (?). My bad for not catching this earlier!
I think the best thing to do for now might be to explicitly pass in shots here, i.e.
result = eng.run(program, shots=program.run_options["shots"])but ideally, I think this line should be eventually modified to only override shots if it is explicitly passed in:
There was a problem hiding this comment.
Thanks @pauljxtan for catching this! Do you want to create a quick PR to master to fix this?
There was a problem hiding this comment.
Sure thing, I will get that done ASAP!
Co-Authored-By: Nathan Killoran <[email protected]> Co-Authored-By: Theodor <[email protected]>
This PR merges the demo branch into master in preparation for release. In particular, it adds the following features which so far have been developed in isolation in the demo branch:
The
ConnectionandJobclassesThe
RemoteEngineThe
sfcommand line interfaceThe
sf.configurationmoduleDocumentation and tests related to the above, including a tutorial
tutorials/remote.rstwalking through usage ofRemoteEngine.While this PR is large, it is composed of code-reviewed PRs, it is still worth giving it a quick once-over.
Note: please have a look at the updated changelog (rendered version here: https://strawberryfields.readthedocs.io/en/demo/development/release_notes.html). I have collated several independent PRs into single bullet points, since this will be squash-merged into master, and added examples.
@nquesada and @thisac, do you have a nice plot we could add to the 'speeding up the fock backend' change?
@trbromley, do you have a nice small example we could add to the 'weighted graphs' change?