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

Further tests and reading loading config checks #89

Merged
merged 20 commits into from
Jul 16, 2021
Merged

Further tests and reading loading config checks #89

merged 20 commits into from
Jul 16, 2021

Conversation

GlaserN
Copy link
Collaborator

@GlaserN GlaserN commented May 17, 2021

add tests transmon_expanded, tf_utils, parametermap features
Enable more tolerances on several tests.

We are now testing at least some asdict and from_config functionality!:

  • asdict of c3objs includes name of object
  • save additional options of the Experiment to the config
  • write more attributes of model to the config
  • Load instruction correctly from dict

transmon_expanded fitting of EJ EC fixed
change functions in tf_utils to be batch compatible and more straight forward
added hjson_encode function. Being able to save some more functionality with hjson, including complex numbers

@GlaserN GlaserN requested a review from fedroy May 17, 2021 15:07
@lazyoracle
Copy link
Member

Is this file redundant (and not where it should be)? - https://github.com/q-optimize/c3/blob/dev/c3/test_transmon_expanded.py

@lazyoracle
Copy link
Member

Tests that use that all_close() and its derivatives should also include an explicit mention of the expected/permissible tolerances based on the test author's assessment of possible numerical artifacts. Otherwise there is no baseline to check against, when these tests inevitably start failing or breaking down the road. A lot of tests have been added in this PR and also in #87 and these should all be updated with explicit tolerance values (atol, rtol) instead of relying on the default numpy figures.

@GlaserN GlaserN marked this pull request as draft May 27, 2021 08:46
@GlaserN
Copy link
Collaborator Author

GlaserN commented Jun 9, 2021

@nwittler is this change for the test in 80d83e3 ok? The Instruction is specified with a target qubit, meaning it is noted in the gateset.

@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #89 (216395c) into dev (8dafe72) will increase coverage by 3.30%.
The diff coverage is 70.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev      #89      +/-   ##
==========================================
+ Coverage   67.51%   70.81%   +3.30%     
==========================================
  Files          36       36              
  Lines        5162     5222      +60     
==========================================
+ Hits         3485     3698     +213     
+ Misses       1677     1524     -153     
Impacted Files Coverage Δ
c3/libraries/propagation.py 30.32% <0.00%> (+4.09%) ⬆️
c3/utils/log_reader.py 0.00% <0.00%> (ø)
c3/utils/utils.py 65.04% <ø> (-2.20%) ⬇️
c3/signal/pulse.py 79.16% <25.00%> (-1.36%) ⬇️
c3/c3objs.py 86.23% <46.51%> (-6.99%) ⬇️
c3/generator/generator.py 94.02% <50.00%> (+0.09%) ⬆️
c3/main.py 56.81% <50.00%> (+0.49%) ⬆️
c3/generator/devices.py 68.23% <62.50%> (+3.39%) ⬆️
c3/model.py 85.84% <70.00%> (+11.96%) ⬆️
c3/utils/tf_utils.py 48.10% <75.00%> (-0.14%) ⬇️
... and 16 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 8dafe72...216395c. Read the comment docs.

@GlaserN
Copy link
Collaborator Author

GlaserN commented Jun 9, 2021

This is currently not the way it is implemented. The main problem was that reading the config file of the Instruction did not include parsing the targets. I guess currently the index is only not necessary to be given if no target is specified.

@GlaserN GlaserN changed the title add more tests Further tests and reading loading config checks Jun 9, 2021
@GlaserN GlaserN marked this pull request as ready for review June 9, 2021 14:37
@nwittler
Copy link
Collaborator

nwittler commented Jun 9, 2021

This is currently not the way it is implemented. The main problem was that reading the config file of the Instruction did not include parsing the targets. I guess currently the index is only not necessary to be given if no target is specified.

You are correct. Also, the drive channels and the target are not necessarily the same thing. You could have a single qubit operation with a correction pulse other qubits for example. That would read

"rx90p[0]", "Q1", ...
"rx90p[0]", "Q2", ...

As we get more QASMy, we can do this properly.

superA = tf.matmul(tf_spre(A), tf_spost(tf.linalg.adjoint(A)))
superA = tf.matmul(
tf_spre(A), tf_spost(tf.linalg.matrix_transpose(A, conjugate=True))
)
return superA
Copy link
Collaborator

Choose a reason for hiding this comment

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

can this be further simplified to do a simply kron(A,A*)?

@lazyoracle
Copy link
Member

@GlaserN @fedroy We are planning to include this PR in the next release (1.3) due at the end of this month, since this would greatly improve the test coverage. Is there any way we can help to complete the pending changes to get this merged in time?

fedroy
fedroy previously approved these changes Jun 30, 2021
Copy link
Member

@lazyoracle lazyoracle left a comment

Choose a reason for hiding this comment

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

test_transmon_expanded.pickle and test_tf_utils.pickle are files that are several MBs in size. Amounting to ~20 MB. This should in general be avoided - adding large binary files to the repository. Is it possible to use a pytest fixture to generate the required data on demand? We want to avoid keeping big binary data files in the code repository as much as possible.

c3/optimizers/c3.py Outdated Show resolved Hide resolved
lazyoracle added a commit that referenced this pull request Jul 3, 2021
Make dependencies less strict
- Less strict requirements with >=
- test/test_envelopes.py has been updated to include a variable atol 
which is calculated based on the desired data. This was outlined in #89 
and the same has been adopted here after some refactoring to allow 
for easy updating and maintenance.
@lazyoracle
Copy link
Member

The updates to test/test_envelopes.py introduced in 702cbdc have now been upstreamed in dev through commits 8cc1441 and 6cf6c1d to allow for merging #95 and fixing #122. It follows the same idea of a variable atol as implemented here, but provides a global constant and a function to make it easy to maintain and update in the future. While resolving merge conflicts of this branch with dev, care should be taken to ensure that the changes in dev in test/test_envelopes.py are not overwritten.

� Conflicts:
�	c3/optimizers/optimalcontrol_robust.py
�	c3/utils/utils.py
�	test/test_envelopes.py
fedroy
fedroy previously approved these changes Jul 15, 2021
@fedroy
Copy link
Collaborator

fedroy commented Jul 15, 2021

test_transmon_expanded.pickle and test_tf_utils.pickle are files that are several MBs in size. Amounting to ~20 MB. This should in general be avoided - adding large binary files to the repository. Is it possible to use a pytest fixture to generate the required data on demand? We want to avoid keeping big binary data files in the code repository as much as possible.

I've made the files smaller, around 1MB max

@fedroy fedroy requested a review from lazyoracle July 15, 2021 22:02
Copy link
Member

@lazyoracle lazyoracle left a comment

Choose a reason for hiding this comment

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

Let's squash and merge this.

@nwittler nwittler merged commit 059f1d1 into dev Jul 16, 2021
@nwittler nwittler deleted the add_tests branch July 16, 2021 09:43
lazyoracle added a commit that referenced this pull request Jul 16, 2021
nwittler pushed a commit to frosati1/c3 that referenced this pull request Jul 23, 2021
* add more tests

* Add save and read test

* fix and add tests

* Save all information of instruction to config  and be able to save slightly more complex json files inncluding ocmplex numbers

* added further decode hooks

* add tolerance bounds for testing

* rewrite spre and spost

* clean merge problems

* Add target qubit in parametermap

* mute unnecessary warning

* remove merge garbage

* flake changes

* black pre-commit adjustments

* removing import incorrectly merged

* made test pickles smaller and updated test to make retaking data easier

Co-authored-by: Federico Roy <federicoaroy@gmail.com>
nwittler pushed a commit to frosati1/c3 that referenced this pull request Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants