Hotfix for minus sign in DRAG shape

[stavros11]

Fixes #826. We were originally planning to postpone this to 0.2, where it is automatically fixed, however since it is relevant for some experiments done now (see qiboteam/qibocal#689 (comment)) and the fix was easy, it would be good to merge it now.

I repeated the experiments from qiboteam/qibocal#689 (review) using Sergi's runcards from the reports uploaded in that comment before and after DRAG calibration. Here are my reports after the sign correction:

Before: http://login.qrccluster.com:9000/2LXRRtnlTV-dnJXpxoN7BQ==
After: http://login.qrccluster.com:9000/Ov8k7evxSfuy7GburXvBiQ==

The difference is small, but I believe DRAG now works more consistently over all qubits (in contrast to before that was only working for qubit 0 for which beta > 0). @igres26 @andrea-pasquale you can also confirm if you like.

(@alecandido maybe you can double check the fix - it is only one line but it's in regex which, as you know, is my favorite!)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.67%. Comparing base (4ae5445) to head (c6945e6).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #869   +/-   ##
=======================================
  Coverage   66.67%   66.67%           
=======================================
  Files          55       55           
  Lines        5902     5902           
=======================================
  Hits         3935     3935           
  Misses       1967     1967           

Flag	Coverage Δ
unittests	66.67% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alecandido]

Premise: this is in eval that is already replaced in 0.2, so if it's passing, I wouldn't care that much.

[abcd] matches any character within (once), and [abcd]+ matches one or more of occurrences of any characters in the list.
Thus having twice \d (a digit) makes no sense.

I assume [...] has been used in place of (...), that is just a group (so the elements inside have to be matched in the order they are written).

\w is a word character (i.e. a digit or letter or _), and I don't see how you need that for the parameters (unless you were writing them as kwargs, but then you'd need also an =), so it's essentially matching just digits, and we're lucky that [...] is simply making it ignored (since \d twice and \w just means \w, inside [], and it's going to match only digits).

[alecandido]

I believe that the pattern was meant to match numbers, and not ,, that are used as a separator.

Thus the pattern should have been:

r"[+-]?\d+(\.\d*)?([eE][+-]?\d+)?"

I.e.:

• either + or - or none of them (but not both)
• then one or more digits
• possibly a . followed by zero or more digits
• possibly a group, led by an e (upper or lower case), with either + or - or none of them (but not both) and then one or more digits

I didn't test it, but if you want to confirm you can look for a float pattern, or just test yourself.

[alecandido]

However, since the idea is just parsing comma-separated float (or integers, that are float compatible), I'd suggest just the following:

shape_parameters = [float(p) for p in value.split(",")]

Unless there are also booleans involved (for which you should try one or the other).

In any case, I'd convert to a programmatic parse (leveraging standard utilities) rather than using regexes...

[stavros11]

Thanks for the summary @alecandido. I actually learnt several things from these comments (which I could most likely also find online, but thanks for summarizing). Generally the idea behind this PR was exactly

Premise: this is in eval that is already replaced in 0.2, so if it's passing, I wouldn't care that much.

so I did a fix that required the minimal effort (and time) and since both pytests and the hardware check seemed to work, I didn't care much more.

Also, note

qibolab/src/qibolab/pulses.py

Line 222 in c6945e6

# TODO: create multiple tests to prove regex working correctly

which probably means that it is known that the current approach is not robust. Also, none of these approaches will work for the Custom shape.

shape_parameters = [float(p) for p in value.split(",")]

Indeed this is cleaner, however I think some additional manipulations are needed, because value also contains the name of the shape. For example value may be Drag(5, 0.02), so we would first need to extract 5, 0.02 (using regex or some other string gymnastics) and then split.

Again, given that this will be irrelevant pretty soon (and also none of these approaches is acceptable for Custom), I would leave as it is, and merge if someone else confirms it fixes the issue in the qibocal discussion.

[alecandido]

I thought that value was only the inside of parentheses, since parentheses otherwise you would match the name as well (but, well, maybe that's what it was desired).

In that case, you could split it in two, and match the name and parameters, and then parse the parameters separately.
Something like:

match_ = re.fullmatch(r"(\w)\(.*\)", value)
name = match_[1]
shape_parameters = [float(p) for p in match_[2].split(",")]

(to be tested, I just wrote it in GitHub).

And yes, not even this one would work for the Custom, but definitely, I wouldn't solve the problem at this level now. And if it's working for what you need, just keep it as it is.

[alecandido]

Given that this is a temporary patch, and only thought to work for the cases we have at hand, I will leave the approval to actual "users" (i.e. @igres26 and @andrea-pasquale)

[andrea-pasquale]

Thank you for the quick fix @stavros11.
The fix was as expected: http://login.qrccluster.com:9000/YTEAvL9UQ5-dG7mHgjGLNQ==

[igres26]

Seems to work well for what I can see. After the comments from @alecandido we can merge.

[alecandido]

Thanks @andrea-pasquale and @igres26, I have no further comment.

@stavros11, you can proceed merging!