Skip to content

Update format of the generator chain #129

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

Merged
merged 12 commits into from
Dec 23, 2021

Conversation

alex-simm
Copy link
Collaborator

@alex-simm alex-simm commented Jul 19, 2021

What

Changes the form of the signal chain that the generator accepts into a dictionary. For each device, the dict specifies which devices are used as inputs, for example:

"d1": {
    "LO": [],
    "AWG": [],
    "DigitalToAnalog": ["AWG"],
    "Response": ["DigitalToAnalog"],
    "Mixer": ["LO", "Response"],
    "VoltsToHertz": ["Mixer"]
}

instead of
"d1": ["LO", "AWG", "DigitalToAnalog", "Response", "Mixer", "VoltsToHertz"]

Also, I added an optional callback to the generator which can be used for debugging during the signal generation.

Why

Closes #29

Previously, the order of the devices in the chain depended on the way the generator internally stored the signals on a stack. The dictionary can be created more intuitively and does not depend on the actual implementation of the generator.

How

The generator brings the directed graph into topological order. Then it iterates through the devices, only storing the signals as long as they are needed. This implementation should be able to handle any signal chain, even if the output of one device is used as input for several others. In case of a cycle, it will raise an error.

Remarks

I didn't test it with complicated signal chains, but some are covered by the unit tests.

Checklist

  • Tests - Added unit tests for new code, regression tests for bugs and updated the integration tests if required
  • Formatting & Linting - black and flake8 have been used to ensure styling guidelines are met
  • Type Annotations - All new code has been type annotated in the function signatures using type hints
  • Docstrings - Docstrings have been provided for functions in the numpydoc style
  • Documentation - The tutorial style documentation has been updated to explain changes & new features
  • Notebooks - Example notebooks have been updated to incorporate changes and new features
  • Changelog - A short note on this PR has been added to the Upcoming Release section

@alex-simm alex-simm marked this pull request as ready for review July 29, 2021 14:23
@alex-simm
Copy link
Collaborator Author

So far, the code is ready and should work. Some of the tests are failing because I used a package from python 3.9. Do we need that backwards compatibility?

@alex-simm alex-simm force-pushed the update-generator-structure branch from 781fe97 to 863469e Compare July 29, 2021 14:31
@lazyoracle
Copy link
Member

So far, the code is ready and should work. Some of the tests are failing because I used a package from python 3.9. Do we need that backwards compatibility?

Yep, we only very recently moved to provide support for Python 3.9 and there is no plan to drop support for 3.6, 3.7 and 3.8 in the upcoming releases. So ideally we should not use any features that are Python 3.9 only

@lazyoracle
Copy link
Member

lazyoracle commented Aug 1, 2021

A possible alternative to consider (which makes for a good choice despite being an external dependency) is networkx. It is widely used, stable, been around for a long time and is also used by other quantum programming toolchains (eg qiskit which also has their own re-implementation in rust retworkx which also has support for Python 3.6). I would suggest we use networkx and drop support for Python 3.6 instead of dropping all previous versions in favour of the still pretty new Python 3.9 which many libraries don't fully support yet.

@shaimach
Copy link
Collaborator

shaimach commented Aug 1, 2021 via email

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2021

CLA assistant check
All committers have signed the CLA.

@alex-simm alex-simm force-pushed the update-generator-structure branch from 4910381 to d720e25 Compare August 2, 2021 20:32
@alex-simm
Copy link
Collaborator Author

I added a simple implementation of topological sorting. This should work without any dependence on python 3.9 now.

@alex-simm alex-simm force-pushed the update-generator-structure branch 2 times, most recently from 08239c7 to f495db7 Compare August 3, 2021 14:10
@alex-simm alex-simm force-pushed the update-generator-structure branch from f495db7 to 67f502b Compare August 3, 2021 14:14
@lazyoracle
Copy link
Member

Putting this on hold until #134 is fixed

Please only update the changelog by adding new sections
and not removing any of the instructions at the top

This reverts commit 863469e.
@lazyoracle
Copy link
Member

Updates to CHANGELOG should follow the style in 4d6c8e2. Sorry if this wasn't adequately clear in the CONTRIBUTING document. The changelog should be updating by adding new details about what was done without removing content from the top.

@lazyoracle lazyoracle modified the milestone: 1.4 Dec 7, 2021
@codecov
Copy link

codecov bot commented Dec 22, 2021

Codecov Report

Merging #129 (7757235) into dev (b0ce40c) will increase coverage by 0.53%.
The diff coverage is 89.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #129      +/-   ##
==========================================
+ Coverage   71.28%   71.81%   +0.53%     
==========================================
  Files          36       36              
  Lines        5237     5475     +238     
==========================================
+ Hits         3733     3932     +199     
- Misses       1504     1543      +39     
Impacted Files Coverage Δ
c3/generator/generator.py 91.00% <89.58%> (-3.03%) ⬇️
c3/experiment.py 78.05% <0.00%> (-4.36%) ⬇️
c3/model.py 84.79% <0.00%> (-1.06%) ⬇️
c3/main.py 57.30% <0.00%> (+0.48%) ⬆️
c3/utils/tf_utils.py 48.73% <0.00%> (+0.63%) ⬆️
c3/libraries/fidelities.py 34.10% <0.00%> (+12.44%) ⬆️
c3/libraries/propagation.py 57.09% <0.00%> (+26.76%) ⬆️

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 b0ce40c...7757235. Read the comment docs.

Copy link
Collaborator

@nwittler nwittler left a comment

Choose a reason for hiding this comment

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

Good to go.

@nwittler nwittler merged commit 2a35b4d into q-optimize:dev Dec 23, 2021
@lazyoracle lazyoracle mentioned this pull request Dec 23, 2021
@lazyoracle lazyoracle added this to the 1.4 milestone Dec 23, 2021
@alex-simm alex-simm deleted the update-generator-structure branch January 19, 2022 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store all signal and improve structuce of signal line
5 participants