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

Split RX from RXY #1050

Merged
merged 8 commits into from
Sep 19, 2024
Merged

Split RX from RXY #1050

merged 8 commits into from
Sep 19, 2024

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Sep 19, 2024

Unfortunately, the RXY name is non-standard, but I was lacking a better one.
Any suggestion? @stavros11 @andrea-pasquale

  • restore some validation?

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.10%. Comparing base (26df7f2) to head (195f9ce).
Report is 10 commits behind head on 0.2.

Additional details and impacted files
@@            Coverage Diff             @@
##              0.2    #1050      +/-   ##
==========================================
- Coverage   52.34%   52.10%   -0.24%     
==========================================
  Files          63       63              
  Lines        2820     2806      -14     
==========================================
- Hits         1476     1462      -14     
  Misses       1344     1344              
Flag Coverage Δ
unittests 52.10% <100.00%> (-0.24%) ⬇️

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.

@andrea-pasquale
Copy link
Contributor

Unfortunately, the RXY name is non-standard, but I was lacking a better one. Any suggestion? @stavros11 @andrea-pasquale

I was discussing with @stavros11 about this yesterday. What about just R or Rn?

@alecandido
Copy link
Member Author

I was discussing with @stavros11 about this yesterday. What about just R or Rn?

To be fair, I'm not even completely sure that it should be a native. I see it better as a compilation rule, since the native should just store the calibrated pulse.

However, by now I'm entering more and more in feature freeze mode. If there is a sufficient agreement about the above statement, it will become an issue for 0.3, and that's it (this "parametrized pi-pulse" is there since months, and I won't change the last day before release).

For me, all names are good enough, just tell me your favorite (one for which there is some consensus), and I can change it.

Base automatically changed from public-api to 0.2 September 19, 2024 13:02
@stavros11
Copy link
Member

I was discussing with @stavros11 about this yesterday. What about just R or Rn?

I would say R is fine. It is simple and doesn't really introduce new physics.

To be fair, I'm not even completely sure that it should be a native. I see it better as a compilation rule, since the native should just store the calibrated pulse.

I actually think the proper solution is this, have only the RX (with fixed amplitude and zero phase) as native. However, the side-effect is that then the user (mainly qibocal) would have to do the pydantic gymnastics (tune the amplitude, change the phase) to get the other rotations. Which is not necessarily bad, we just need to get used to it.

Generally, I would only call natives what we are actually calibrating. If we are calibrating the RX90 directly I would then call it native, but unfortunately I don't think we have support for that in 0.2, native kinds are fixed. If we need to further manipulations to get other gates, I would do them outside the Platform.

@alecandido
Copy link
Member Author

I would say R is fine. It is simple and doesn't really introduce new physics.

Ah, wait, maybe that name is the one already used:
https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.RGate

@alecandido
Copy link
Member Author

I actually think the proper solution is this, have only the RX (with fixed amplitude and zero phase) as native. However, the side-effect is that then the user (mainly qibocal) would have to do the pydantic gymnastics (tune the amplitude, change the phase) to get the other rotations. Which is not necessarily bad, we just need to get used to it.

Generally, I would only call natives what we are actually calibrating. If we are calibrating the RX90 directly I would then call it native, but unfortunately I don't think we have support for that in 0.2, native kinds are fixed. If we need to further manipulations to get other gates, I would do them outside the Platform.

I perfectly agree with you, but I'd also like to save whichever user the gymnastics you mentioned.
Since we already have a proposal to expose the compiler in a smoother way #1042, let's first do that, and then we will reconsider also this method.

@alecandido alecandido added this to the Qibolab 0.2.0 milestone Sep 19, 2024
@alecandido
Copy link
Member Author

  • restore some validation?

There is no strong reason why single qubit gates should be single pulses or applied on individual channels.
True, RX is usually the pi-pulse. But if any user wanted to add something else, it's not necessary to deny it.

Let's just leave it to users' responsibility.

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thanks @alecandido, looks good to me.


def create_sequence(self) -> PulseSequence:
return deepcopy(self)
``theta`` will be the angle of the rotation, while ``phi`` the angle that the rotation axis forms with x axis.
Copy link
Member

Choose a reason for hiding this comment

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

In principle, we could leave the Args: structure in the docstring here, as it was before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we could.

This is something I'm doing on my own: frequently the Args: (and equivalents) contain redundant information (essentially repeating the variable names), and redundant type hints.

It was not the case of this docstring in particular, as it is essentially unchanged. But I'm trying to keep the style more discursive.
However, I'm not pushing to enforce it. Not now, at least.

@alecandido
Copy link
Member Author

@andrea-pasquale is it fine for you like this?

Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

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

Yes, fine by me. Just a tiny suggestion.

src/qibolab/_core/native.py Outdated Show resolved Hide resolved
@alecandido
Copy link
Member Author

I'm merging as soon as the CI will pass

@alecandido alecandido merged commit 6fbf53f into 0.2 Sep 19, 2024
28 checks passed
@alecandido alecandido deleted the isolate-pi-pulse branch September 19, 2024 16:43
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.

3 participants