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

Adding circuits with the same name does not yield a circuit with the same name #5889

Closed
quantumjim opened this issue Feb 23, 2021 · 13 comments
Closed
Labels
type: enhancement It's working, but needs polishing

Comments

@quantumjim
Copy link
Member

What is the expected enhancement?

When adding two circuits with the same name, I'd expect that name to carry over to the new circuit. But that doesn't happen

qc1 = QuantumCircuit(1)
qc2 = QuantumCircuit(1)

qc1.name = 'test'
qc2.name = 'test'

assert (qc1+qc2).name=='test'

Furthermore, for adding circuits without different names, I'd expect the new circuit to have the name of the first one.

In any case, some consistent behaviour beyond the arbitrary generation of a new name would be nice.

@quantumjim quantumjim added the type: enhancement It's working, but needs polishing label Feb 23, 2021
@1ucian0 1ucian0 added the good first issue Good for newcomers label Feb 24, 2021
@1ucian0
Copy link
Member

1ucian0 commented Feb 24, 2021

if they have different names (eg name1 and name2), would something like name1_name2 make sense as the result?

@Cryoris
Copy link
Contributor

Cryoris commented Feb 24, 2021

I don't think adding two circuits should combine names. If I stack together my circuit for an application out of many many small circuits the name would be huge and probably not very meaningful.

If you want to preserve the name, could you not instead use

qc1.name = 'test'
qc1.compose(qc2, inplace=True)

or

added = QuantumCircuit(n, name='test')
added.append(qc1, range(n))
added.append(qc2, range(n))

?

@boschmitt
Copy link
Contributor

I agree with @Cryoris.

If you create a new circuit, then I don't see any reason why you should favor naming it after the first circuit over the second when doing qc1 + qc2. Furthermore, if we chose to name after the first one, then we would end up with a third circuit qc3 which has the same name as qc1 but is different.

@quantumjim
Copy link
Member Author

When I add circuits, it is because I want some additional gates from the second circuit to be appended to my main circuit. So it makes sense to me for the sum to have the same name as the first term. I suppose the compose option that @Cryoris mentioned is the best practice way to make sure the name is inherited for this specific use case.

Nevertheless, I guess we have should some expectation of how and why people will + for circuits, and should have a consistent rule for combining user-defined names based on that expected usage rather than just replacing them with something random. The concatenation suggested by @1ucian0 makes sense to me. I agree that it could get unwieldy, but with all the info that exists within a qobj, would a long name even be a drop in the ocean?

@boschmitt
Copy link
Contributor

When I add circuits, it is because I want some additional gates from the second circuit to be appended to my main circuit.

Yes, but there is a difference between adding to a circuit qc1 += qc2 and adding two circuits qc1 + qc2. The later creates a third circuit that is neither qc1 nor qc2. By using +, the user is signalling the intent of creating this new circuit. Thus, I don't think it should inherit any name.

The concatenation suggested by @1ucian0 makes sense to me. I agree that it could get unwieldy, but with all the info that exists within a qobj, would a long name even be a drop in the ocean?

It makes sense, but I would argue that it is pointless. I think that when using +, the user wants to create a new circuit that it's either temporary or "final"/main. In the first case, the name doesn't matter. In the second, the user should know a better name than the concatenation and should manually change it.

@1ucian0 1ucian0 removed the good first issue Good for newcomers label Feb 25, 2021
@1ucian0
Copy link
Member

1ucian0 commented Feb 25, 2021

Let's survey it (non-binding, just to see what's the general feeling):

if qc1.name == name1 and qc2.name == name2, then (qc1+qc2).name should be:

  • <some random>. If you like this option, react with 🚀
  • name1_name2. If you like this option, react with 👀
  • name1. If you like this option, react with ❤️

@HuangJunye
Copy link
Collaborator

HuangJunye commented Feb 25, 2021

This is fun. I personally never used qc.name. So for me it doesn't matter. I think a random name is fine. If the user want to keep name 1 or name 2, they can just specify it. Like @boschmitt said, qc1 + qc2 is an entire new circuit so it should be just the same as you create a new QuantumCircuit object which I just tested is circuitx where x is a number.

@nonhermitian
Copy link
Contributor

So there is more to this then simple issues like naming a new Python class instance. In particular, appending names together gives one an indication of the composite structure of the combined circuit. This information is useful. Going further, this information is critical for building composite circuits where one of the sub-blocks has a known optimal representation, and should not be modified, or perhaps represented as an opaque circuit. I would not use the name for this, but the point is that the composite info is important. At present, the name is the only place that can include this and as such the appending circuit names seems to be a reasonable thing to do.

@mtreinish
Copy link
Member

So there is more to this then simple issues like naming a new Python class instance. In particular, appending names together gives one an indication of the composite structure of the combined circuit. This information is useful. Going further, this information is critical for building composite circuits where one of the sub-blocks has a known optimal representation, and should not be modified, or perhaps represented as an opaque circuit. I would not use the name for this, but the point is that the composite info is important. At present, the name is the only place that can include this and as such the appending circuit names seems to be a reasonable thing to do.

Well there is the metadata attribute on circuits now, we could have compose populate the metadata of the output circuit with more detailed information as needed.

@nonhermitian
Copy link
Contributor

Well there is the metadata attribute on circuits now, we could have compose populate the metadata of the output circuit with more detailed information as needed.

Yes, that could also work in this case. That would need to be flushed out a bit more to work in the larger context though.

@kdk
Copy link
Member

kdk commented Feb 25, 2021

Agree with @nonhermitian that name has an unexpectedly important and widespread role throughout terra, and changes to its behavior should be made thoughtfully.

That said, QuantumCircuit.{extend, combine} (and their current aliases as + and +=) are slated for deprecation in the next terra release ( see #4208 ) to be replaced in the future with QuantumCircuit.compose and .compose(inplace=True) (which currently both have the "keep the name of the LHS" behavior) so any potential changes should be considered in that light.

@pdc-quantum
Copy link
Member

pdc-quantum commented Feb 27, 2021

I feel that "If the user want to keep name 1 or name 2, they can just specify it" is a strong argument from @HuangJunye .
More generally if users can (re)name the newborn circuit as they wish, the default name should be random to avoid confusion with any other circuit.

@kdk
Copy link
Member

kdk commented Jun 20, 2023

Closing as QuantumCircuit.{extend, combine} (and their + and += aliases) have been removed in favor of .compose and .compose(inplace=True) which have the requested behavior.

@kdk kdk closed this as completed Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement It's working, but needs polishing
Projects
None yet
Development

No branches or pull requests

9 participants