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

Data preparation initializer.py is calling a method on StatePreparation that no longer exists #12969

Closed
woodsp-ibm opened this issue Aug 16, 2024 · 5 comments · Fixed by #12976
Assignees
Labels
bug Something isn't working mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library

Comments

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Aug 16, 2024

Environment

  • Qiskit version: 1.2
  • Python version: [3.8, 3.12]
  • Operating system: Linux, Windows, Mac

What is happening?

Given 1.2 just released I quickly looked around the application modules just as a quick check all was good (algorithms runs tests against qiskit main so as to get some heads up, but the apps just run/test off stable release) - Qiskit Finance nightly CI tests failed in several of them due to this:

      File "/home/runner/work/qiskit-finance/qiskit-finance/qiskit_finance/circuit/library/probability_distributions/normal.py", line 218, in __init__
    circuit = initialize.gates_to_uncompute().inverse()

      File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/qiskit/circuit/library/data_preparation/initializer.py", line 91, in gates_to_uncompute
    return self._stateprep._gates_to_uncompute()

    AttributeError: 'StatePreparation' object has no attribute '_gates_to_uncompute'

return self._stateprep._gates_to_uncompute()

How can we reproduce the issue?

Create an initializer Initialize instance and call its method gates_to_uncompute

What should happen?

The gates_to_uncompute public method should work, as it used to, even if internal implementation is altered.

The change, where the method that initializer was calling got removed, seems to have come in #12178

Any suggestions?

No response

@woodsp-ibm woodsp-ibm added the bug Something isn't working label Aug 16, 2024
@ShellyGarion ShellyGarion self-assigned this Aug 18, 2024
@ShellyGarion
Copy link
Member

Thank you @woodsp-ibm for detecting this bug. Could you please check if #12976 fixes it?

@alexanderivrii
Copy link
Contributor

Thanks @ShellyGarion. Looking at the code, I believe that the fix should work (though let's wait for the confirmation from @woodsp-ibm).

I do have one one possibly naive question: the qiskit-finance code calls

# use default the isometry (or initialize w/o resets) algorithm to construct the circuit
if upto_diag:
    inner.append(Isometry(np.sqrt(normalized_probabilities), 0, 0), inner.qubits)
    self.append(inner.to_instruction(), inner.qubits)  # Isometry is not a Gate
else:
    initialize = Initialize(np.sqrt(normalized_probabilities))
    circuit = initialize.gates_to_uncompute().inverse()
    inner.compose(circuit, inplace=True)
    self.append(inner.to_gate(), inner.qubits)

If I understand correctly, initialize.gates_to_uncompute().inverse() amounts to calling Isometry(...) and applying inverse() twice in a row. Would it make sense to just call Isometry(...) and then maybe we don't even need the if -- else? I actually also don't understand how exactly up_to_diag is used when calling the Isometry code.

Of course, this does not invalidate the PR, as we still need to ensure that the publicly documented API function Initialize.gates_to_uncompute() works correctly.

@ShellyGarion
Copy link
Member

The original synthesis code (before it was replaced by Isometry) was:

       # call to generate the circuit that takes the desired vector to zero
        disentangling_circuit = self._gates_to_uncompute()
              if self._inverse is False:
            initialize_instr = disentangling_circuit.to_instruction().inverse()
        else:
            initialize_instr = disentangling_circuit.to_instruction()

        q = QuantumRegister(self.num_qubits, "q")
        initialize_circuit = QuantumCircuit(q, name="init_def")
        initialize_circuit.append(initialize_instr, q[:])
        return initialize_circuit

So, it basically only does _gates_to_uncompute()

@Cryoris
Copy link
Contributor

Cryoris commented Aug 19, 2024

If I understand correctly, initialize.gates_to_uncompute().inverse() amounts to calling Isometry(...) and applying inverse() twice in a row. Would it make sense to just call Isometry(...) and then maybe we don't even need the if -- else

The problem was that Isometry is a Instruction but we needed a Gate, since we later on used the circuit as building block and required e.g. controlling or computing inverses. But at that point StatePreparation did not exist if I recall correctly, and we should probably change to using that class in Qiskit Finance.

@ShellyGarion ShellyGarion added the mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library label Aug 19, 2024
@woodsp-ibm
Copy link
Member Author

Thank you @woodsp-ibm for detecting this bug. Could you please check if #12976 fixes it?

@ShellyGarion I copied the initializer.py from your branch into my local install off Qiskit main and with that tests that failed before now pass. So yes it fixes things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants