Change rep_time unit to s #3612

eembees · 2019-12-13T16:52:48Z

Summary

-Changed the rep_time unit to be in seconds, not microseconds.
-Updated tests to reflect scaling of the rep_time variable
Fixes #3598

Details and comments

rep_times can still be given in microseconds, but is then scaled down to s. However, the rep_times need to be given as floats, not as ints as before.

added one time warning for rep_time Fixes Qiskit#3598

claassistantio · 2019-12-13T16:57:40Z

All committers have signed the CLA.

claassistantio · 2019-12-13T16:57:42Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

test/python/qobj/test_qobj.py

now passes all unit tests

eembees · 2019-12-23T06:41:14Z

@taalexander pinging you here as well 😄

lcapelluto

thank you for taking this on! sorry that we have been MIA for a bit

qiskit/providers/models/backendconfiguration.py

qiskit/compiler/assemble.py

qiskit/providers/models/backendconfiguration.py

qiskit/qobj/models/pulse.py

qiskit/schemas/examples/backend_configuration_openpulse_example.json

qiskit/test/mock/fake_openpulse_3q.py

eembees · 2020-01-07T08:53:24Z

Ah, thank you so much for your feedback @lcapelluto, I will get to work on it asap and hopefully have a new PR by the weekend!
Just to clarify then:

assemble() should take a rep_times as a float (seconds) but pass it on as an int (microseconds) when we build the Qobj ( just like in Fix bug where lo units were not being properly converted #3597 )
all JSONs should be reverted (?)
only tests that call assemble() should be changed to have units of seconds
I will assume the above is correct and start working on this. Correct me if I am wrong, please 😄

… in `assemble()`

added 'rep_time' name to 'BackendDefault' named tuple

lcapelluto

Nice updates! This looks right to me, just needs a few new tests.

test/python/compiler/test_assembler.py

qiskit/compiler/assemble.py

Co-Authored-By: Lauren Capelluto <laurencapelluto@gmail.com>

…change_rep_time_unit_to_s

lcapelluto

I had one suggestion, but still approving :D

lcapelluto · 2020-01-22T22:13:11Z

@eembees Would you mind resolving conflicts? Then I will get my colleagues to get approvals for you

taalexander

LGTM

mtreinish

I think this is fine considering all the other unit changes we've been making on pulse. But before we can move forward with this can you please add a release note to document this change? Since it could catch end users by surprise. The process for doing that is documented here: https://github.com/Qiskit/qiskit-terra/blob/master/CONTRIBUTING.md#release-notes

release note added!

mtreinish

LGTM, thanks for updating, just two small nits inline. Not worth blocking over though I can update things when I go through the release notes pre-release

mtreinish · 2020-01-27T17:17:39Z

releasenotes/notes/change-rep-time-unit-48533d8fe474a035.yaml

+    `PulseBackendConfiguration._parse_pulse_args()` still takes `rep_times` in units of microseconds,
+    but they are now transformed to units of seconds before being passed to `assemble()`
+    At first pass, `PulseBackendConfiguration` will raise a warning to the user.
+deprecations:


This should probably be an upgrade note since it's not something that users have to update now:

Suggested change

deprecations:

upgrade:

mtreinish · 2020-01-27T17:19:43Z

releasenotes/notes/change-rep-time-unit-48533d8fe474a035.yaml

+    At first pass, `PulseBackendConfiguration` will raise a warning to the user.
+deprecations:
+  - |
+    `assemble()` now takes `rep_time` in units of seconds, not microseconds.


We probably should put the full callable path here so that it's clear which function we're talking about:

Suggested change

`assemble()` now takes `rep_time` in units of seconds, not microseconds.

``qiskit.compiler.assemble()`` now takes `rep_time` in units of seconds, not microseconds.

* changed rep_time from microseconds to seconds. added one time warning for rep_time Fixes Qiskit#3598 * fixed docstring * changed so that rep_time only can be a float. now passes all unit tests * reverted all jsons, made rep_time conversion at `_parse_pulse_args()` in `assemble()` * Update assemble.py added 'rep_time' name to 'BackendDefault' named tuple * Update qiskit/compiler/assemble.py Co-Authored-By: Lauren Capelluto <laurencapelluto@gmail.com> * added a test in test_backendconfiguration * linting and removed redundant arg in `assemble.py` * linting * more linting * removed rep_time from BackendDefault tuple * fixed parametric pulse error & lint * release note Co-authored-by: Lauren Capelluto <laurencapelluto@gmail.com> Co-authored-by: Thomas Alexander <thomasalexander2718@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

eembees added 2 commits December 13, 2019 17:44

changed rep_time from microseconds to seconds.

1968bca

added one time warning for rep_time Fixes Qiskit#3598

fixed docstring

30ffc96

eembees requested review from 1ucian0, ajavadia, diego-plan9, kdk, mtreinish and taalexander as code owners December 13, 2019 16:52

eembees force-pushed the change_rep_time_unit_to_s branch from 91706c9 to 39bccc0 Compare December 13, 2019 16:56

lcapelluto reviewed Dec 16, 2019

View reviewed changes

test/python/qobj/test_qobj.py Outdated Show resolved Hide resolved

changed so that rep_time only can be a float.

952e2fa

now passes all unit tests

eembees force-pushed the change_rep_time_unit_to_s branch from 39bccc0 to 952e2fa Compare December 23, 2019 06:35

lcapelluto self-assigned this Jan 6, 2020

lcapelluto suggested changes Jan 6, 2020

View reviewed changes

eembees added 2 commits January 7, 2020 11:12

reverted all jsons, made rep_time conversion at _parse_pulse_args()…

9990898

… in `assemble()`

Update assemble.py

9ebb033

added 'rep_time' name to 'BackendDefault' named tuple

lcapelluto suggested changes Jan 7, 2020

View reviewed changes

test/python/compiler/test_assembler.py Outdated Show resolved Hide resolved

qiskit/compiler/assemble.py Show resolved Hide resolved

eembees and others added 4 commits January 8, 2020 12:22

Update qiskit/compiler/assemble.py

198be3e

Co-Authored-By: Lauren Capelluto <laurencapelluto@gmail.com>

added a test in test_backendconfiguration

d4ac034

Merge remote-tracking branch 'origin/change_rep_time_unit_to_s' into …

f300094

…change_rep_time_unit_to_s

Merge branch 'master' into change_rep_time_unit_to_s

16086c9

eembees requested a review from lcapelluto January 8, 2020 12:20

eembees added 3 commits January 8, 2020 13:30

linting and removed redundant arg in assemble.py

c610c84

linting

6e7374f

more linting

5b096af

lcapelluto approved these changes Jan 9, 2020

View reviewed changes

lcapelluto previously approved these changes Jan 15, 2020

View reviewed changes

Merge branch 'master' into change_rep_time_unit_to_s

1226f9b

eembees dismissed lcapelluto’s stale review via 1226f9b January 23, 2020 09:42

eembees added 2 commits January 23, 2020 11:13

fixed parametric pulse error & lint

ef2a7fe

Merge branch 'master' into change_rep_time_unit_to_s

50f0dcc

taalexander previously approved these changes Jan 23, 2020

View reviewed changes

lcapelluto previously approved these changes Jan 24, 2020

View reviewed changes

lcapelluto added the automerge label Jan 24, 2020

mergify bot and others added 2 commits January 24, 2020 20:10

Merge branch 'master' into change_rep_time_unit_to_s

9d2def3

Merge branch 'master' into change_rep_time_unit_to_s

b30fe9f

mtreinish previously requested changes Jan 24, 2020

View reviewed changes

mergify bot and others added 2 commits January 24, 2020 21:44

Merge branch 'master' into change_rep_time_unit_to_s

a65035d

release note

1e05742

eembees dismissed stale reviews from lcapelluto and taalexander via 1e05742 January 27, 2020 08:12

lcapelluto approved these changes Jan 27, 2020

View reviewed changes

mtreinish reviewed Jan 27, 2020

View reviewed changes

mergify bot added 4 commits January 27, 2020 18:25

Merge branch 'master' into change_rep_time_unit_to_s

18bd524

Merge branch 'master' into change_rep_time_unit_to_s

2c72b47

Merge branch 'master' into change_rep_time_unit_to_s

e2bdd12

Merge branch 'master' into change_rep_time_unit_to_s

aebe595

lcapelluto assigned mtreinish Jan 28, 2020

mergify bot added 2 commits January 28, 2020 21:54

Merge branch 'master' into change_rep_time_unit_to_s

365e44a

Merge branch 'master' into change_rep_time_unit_to_s

16a1193

mtreinish approved these changes Jan 29, 2020

View reviewed changes

mergify bot merged commit e07a2e2 into Qiskit:master Jan 29, 2020

lcapelluto added the Changelog: API Change Include in the "Changed" section of the changelog label Feb 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change rep_time unit to s #3612

Change rep_time unit to s #3612

eembees commented Dec 13, 2019

claassistantio commented Dec 13, 2019 •

edited

Loading

claassistantio commented Dec 13, 2019

eembees commented Dec 23, 2019

lcapelluto left a comment

eembees commented Jan 7, 2020

lcapelluto left a comment

lcapelluto left a comment

lcapelluto commented Jan 22, 2020

taalexander left a comment

mtreinish left a comment

mtreinish left a comment

mtreinish Jan 27, 2020

mtreinish Jan 27, 2020

	`assemble()` now takes `rep_time` in units of seconds, not microseconds.
	``qiskit.compiler.assemble()`` now takes `rep_time` in units of seconds, not microseconds.

Change rep_time unit to s #3612

Change rep_time unit to s #3612

Conversation

eembees commented Dec 13, 2019

Summary

Details and comments

claassistantio commented Dec 13, 2019 • edited Loading

claassistantio commented Dec 13, 2019

eembees commented Dec 23, 2019

lcapelluto left a comment

Choose a reason for hiding this comment

eembees commented Jan 7, 2020

lcapelluto left a comment

Choose a reason for hiding this comment

lcapelluto left a comment

Choose a reason for hiding this comment

lcapelluto commented Jan 22, 2020

taalexander left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Jan 27, 2020

Choose a reason for hiding this comment

mtreinish Jan 27, 2020

Choose a reason for hiding this comment

claassistantio commented Dec 13, 2019 •

edited

Loading