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.

Sign up for GitHub

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

Jump to bottom

Hotfix for minus sign in DRAG shape #869

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Hotfix for minus sign in DRAG shape #869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/qibolab/pulses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ def eval(value: str) -> "PulseShape":
shape_name = re.findall(r"(\w+)", value)[0]
if shape_name not in globals():
raise ValueError(f"shape {value} not found")
shape_parameters = re.findall(r"[\w+\d\.\d]+", value)[1:]
shape_parameters = re.findall(r"[-\w+\d\.\d]+", value)[1:]
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

@stavros11 stavros11 Apr 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

# TODO: create multiple tests to prove regex working correctly
return globals()[shape_name](*shape_parameters)

Expand Down
10 changes: 8 additions & 2 deletions tests/test_pulses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,12 +275,18 @@ def test_pulses_pulseshape_sampling_rate(shape):
def test_pulseshape_eval():
shape = PulseShape.eval("Rectangular()")
assert isinstance(shape, Rectangular)
shape = PulseShape.eval("Drag(5, 1)")
assert isinstance(shape, Drag)
with pytest.raises(ValueError):
shape = PulseShape.eval("Ciao()")


@pytest.mark.parametrize("rel_sigma,beta", [(5, 1), (5, -1), (3, -0.03), (4, 0.02)])
def test_drag_shape_eval(rel_sigma, beta):
shape = PulseShape.eval(f"Drag({rel_sigma}, {beta})")
assert isinstance(shape, Drag)
assert shape.rel_sigma == rel_sigma
assert shape.beta == beta


def test_raise_shapeiniterror():
shape = Rectangular()
with pytest.raises(ShapeInitError):
Expand Down
Loading