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

Add gate labels #853

Merged
merged 16 commits into from
Apr 24, 2023
Merged

Add gate labels #853

merged 16 commits into from
Apr 24, 2023

Conversation

vodovozovaliza
Copy link
Contributor

@vodovozovaliza vodovozovaliza commented Apr 14, 2023

Related to issue #850. This PR allows to change name parameter of qibo.gates.Gate and 2 other labels are added:

  • name: label of the gate. Can be changed by a user.
  • draw_label: a symbol for drawing a circuit. If None or empty, gate.name[:4] is used.
    This parameter allows to remove qibo.gates.DRAW_LABELS dictionary.
  • @property qasm_label: for gates supported by OpenQASM.

For example:

from math import pi 
from qibo import gates
from qibo.models import Circuit

rx1 = gates.RX(0, pi/2)
rx1.name = "RX90"
rx1.draw_label = "RX1"
rx2 = gates.RX(1, 3 * pi/2)
rx2.name = "RX270"
rx2. draw_label = "RX2"

circuit = Circuit(2)
circuit.add(rx1)
circuit.add(rx2)
print(circuit.draw())

prints

q0: ─RX1q1: ─RX2

instead of

q0: ─RXq1: ─RX

As mentioned in #850, the name can be used for NoiseModels or for controlling parameters of the quantum hardware.

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@vodovozovaliza vodovozovaliza added the enhancement New feature or request label Apr 14, 2023
@vodovozovaliza vodovozovaliza self-assigned this Apr 14, 2023
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (532c328) 100.00% compared to head (55681ea) 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##            master      #853    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           48        48            
  Lines         6169      6309   +140     
==========================================
+ Hits          6169      6309   +140     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

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

Impacted Files Coverage Δ
src/qibo/gates/abstract.py 100.00% <100.00%> (ø)
src/qibo/gates/channels.py 100.00% <100.00%> (ø)
src/qibo/gates/gates.py 100.00% <100.00%> (ø)
src/qibo/gates/measurements.py 100.00% <100.00%> (ø)
src/qibo/gates/special.py 100.00% <100.00%> (ø)
src/qibo/models/circuit.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@renatomello renatomello added this to the Qibo 0.1.13 milestone Apr 15, 2023
@renatomello renatomello linked an issue Apr 15, 2023 that may be closed by this pull request
@renatomello renatomello self-requested a review April 17, 2023 09:25
@renatomello
Copy link
Contributor

renatomello commented Apr 17, 2023

the parameters label and name seem to be somewhat redundant. Maybe there's a way to eliminate the redundancy by using class type instead of name, and then using name as label is being intended.

For instance, any time that if self.name == "<insert-name>" appears, we could use if isinstance(self, "insert class type") is True. Then, self.name is free to be used as a label.

I haven't looked at every instance in which self.name is used, so I don't know if this suggestion even works and/or would break other things.

@stavros11
Copy link
Member

the parameters label and name seem to be somewhat redundant. Maybe there's a way to eliminate the redundancy by using class type instead of name, and then using name as label is being intended.

Looking at the changes, I also agree that we could keep only one attribute for label and name (feel free to choose whichever you prefer).

For instance, any time that if self.name == "<insert-name>" appears, we could use if isinstance(self, "insert class type") is True. Then, self.name is free to be used as a label.

I haven't looked at every instance in which self.name is used, so I don't know if this suggestion even works and/or would break other things.

I also do not recall all the places where name is used, but most likely should be possible to replace it with isinstance. @vodovozovaliza if you also agree with us, try to have a look if you can make this change and use the name as label. I think you can easily find all lines that name appears in the code using some grep command in linux.

@vodovozovaliza
Copy link
Contributor Author

@renatomello @stavros11 thank you for the comments, I will look into replacing name and label with just one of them.

@vodovozovaliza
Copy link
Contributor Author

At the moment, gate.name corresponds to its OpenQASM operation. To make this parameter flexible, I would like to propose the following attributes for the gate objects:

  • name: label of the gate. Can be changed by a user.
  • draw_label: a symbol for drawing a circuit. If None or empty, gate.name[:4] is used.
    This parameter allows to remove qibo.gates.DRAW_LABELS dictionary.
  • qasm_label: exists only for gates supported by OpenQASM.

@stavros11
Copy link
Member

Thanks a lot for checking, I agree for the proposal.

  • name: label of the gate. Can be changed by a user.

Feel free to rename this label if you prefer, to match the original proposal for this.

  • draw_label: a symbol for drawing a circuit. If None or empty, gate.name[:4] is used.
    This parameter allows to remove qibo.gates.DRAW_LABELS dictionary.

Indeed, removing DRAW_LABELS is something I also wanted to do for quite some time. It would be great if you can do that.

  • qasm_label: exists only for gates supported by OpenQASM.

Maybe you could make this a @property and raise an error if the gate is not supported by OpenQASM, eg:

class Gate:
    @property
    def qasm_label(self):
        raise_error(NotImplementedError, f"{self.__class__.__name__} is not supported by OpenQASM")

class H(Gate):
    @property
    def qasm_label(self):
        return "h"

In principle we could also consider removing QASM_GATES but this would require modifying a few things in Circuit so we could leave for another PR.

@vodovozovaliza
Copy link
Contributor Author

Thank you @stavros11. Turning qasm_label into @property is a very good idea.

@renatomello
Copy link
Contributor

is this dictionary

QASM_GATES = {
still needed?

@vodovozovaliza
Copy link
Contributor Author

is this dictionary

QASM_GATES = {

still needed?

Yes, this dictionary is needed for Circuit.from_qasm method

@renatomello
Copy link
Contributor

@stavros11 could you also check this before we merge?

@renatomello
Copy link
Contributor

@vodovozovaliza since name became the de facto label, could you rename this PR?

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 for the updates, looks good to me too. I just left a comment regarding removing the PARAMETRIZED_GATES set. This is not strictly required to merge but since you already cleaned the other dictionaries in gates.py, it may be a good idea to do so.

If understand correctly now the user is able to control both the name and the draw_label seperately. Is this okay for your applications or you prefer the draw_label to change every time the name changes? If that's the case we may need to have a setter or drop completely the draw_label and use only the name (but then we lose the flexibility of adjusting them seperately).

src/qibo/models/circuit.py Outdated Show resolved Hide resolved
src/qibo/models/circuit.py Outdated Show resolved Hide resolved
tests/test_models_circuit.py Outdated Show resolved Hide resolved
@vodovozovaliza
Copy link
Contributor Author

Thanks for the updates, looks good to me too. I just left a comment regarding removing the PARAMETRIZED_GATES set. This is not strictly required to merge but since you already cleaned the other dictionaries in gates.py, it may be a good idea to do so.

Thank you for this comment, I removed PARAMETRIZED_GATES as well.

If understand correctly now the user is able to control both the name and the draw_label seperately. Is this okay for your applications or you prefer the draw_label to change every time the name changes? If that's the case we may need to have a setter or drop completely the draw_label and use only the name (but then we lose the flexibility of adjusting them seperately).

The purpose of name parameter is to distinguish objects of the same type (e.g. "KrausChannel1", "KrausChannel2"), so Circuit.draw() result will not always look good if we only use a name (or part of it). If a user sets draw_label to None or an empty string "", name[:4] will be used automatically.
If you believe that it is better to remove 'draw_label, we can find a way of using name` instead, e.g. by extracting capital letters from the name

draw_label = "".join([c for c in gate.name if c.isupper() or c.isdigit()][:4])
if len(draw_label) == 0:
    draw_label = gate.name[:4].upper()

# "KrausChannel1" -> "KC1"
# "x" -> "X"

However, I think it is good to have the flexibility of controlling name and draw_label separately. For instance, SWAP ('x') and CNOT ('X') symbols are different from their names by default and a user can choose to only change the name or both parameters.

@stavros11
Copy link
Member

Thanks for the explanation and update. This was mostly a question towards the users of these features, to me the current solution is fine as it is the most flexible. I agree with merging this.

@renatomello renatomello added this pull request to the merge queue Apr 24, 2023
Merged via the queue into master with commit fcc8232 Apr 24, 2023
@renatomello renatomello deleted the gate_label branch April 24, 2023 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding labels to Qibo gates
3 participants