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

Deprecate StarAlgebraMixin and dagger #210

Merged
merged 11 commits into from
Jun 4, 2021
Merged

Deprecate StarAlgebraMixin and dagger #210

merged 11 commits into from
Jun 4, 2021

Conversation

ikkoham
Copy link
Contributor

@ikkoham ikkoham commented May 26, 2021

Summary

Update internal API.

Related to: #117

I also deprecated the dagger: Qiskit/qiskit#6423 (comment)

Details and comments

mrossinek
mrossinek reviewed May 26, 2021
View reviewed changes
Copy link
Member

@mrossinek mrossinek left a comment

Choose a reason for hiding this comment

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

Thanks @ikkoham! Can you also include EvolvedOpAnsatz or do you want to handle that in a separate PR?

@woodsp-ibm
Copy link
Member

Since this change requires what is in Terra main, i,e, version 0.18.0 when its released, I thin we should update the requirements.txt file to change this qiskit-terra>=0.17.0 to qiskit-terra>=0.18.0

@ikkoham ikkoham changed the title Remove StarAlgebraMixin and deprecate dagger Deprecate StarAlgebraMixin and deprecate dagger May 27, 2021
@ikkoham ikkoham changed the title Deprecate StarAlgebraMixin and deprecate dagger Deprecate StarAlgebraMixin and dagger May 27, 2021
@ikkoham
Copy link
Contributor Author

ikkoham commented May 27, 2021

@mrossinek Thanks.

Can you also include EvolvedOpAnsatz or do you want to handle that in a separate PR?

I think it is impossible in the current stage.
Simple replacement of import path does not work.
This is the issue Qiskit/qiskit#6425.

@woodsp-ibm Thank you for your review. I've updated.

@mrossinek
Copy link
Member

I think it is impossible in the current stage.

Hm that is odd. @Cryoris can you check this, then? Probably in another PR in this case..

@ikkoham
Copy link
Contributor Author

ikkoham commented May 31, 2021

Note about EvolvedOpAnsatz: I said it doesn't work, but to be precise, it doesn't pass the test. I think the problem is that the order of the parameters has changed, or the bounds have changed, so the convergence value has changed.

@pbark pbark merged commit 08f6dce into qiskit-community:main Jun 4, 2021
@mrossinek mrossinek mentioned this pull request Jun 4, 2021
Closed
@woodsp-ibm woodsp-ibm mentioned this pull request Jun 4, 2021
Merged
@ikkoham ikkoham deleted the port-star-algebra branch August 4, 2021 14:02
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request May 25, 2023
@ikkoham @woodsp-ibm @mrossinek
Deprecate StarAlgebraMixin and dagger (qiskit-community#210)
ffb5714 
* remove StarAlgebraMixin and deprecate dagger

* Update qiskit_nature/operators/second_quantization/second_quantized_op.py

Co-authored-by: Steve Wood <[email protected]>

* Update test/operators/second_quantization/test_fermionic_op.py

* Revert StarAlgebraMixin and deprecate it. Change Terra version

Co-authored-by: Steve Wood <[email protected]>
Co-authored-by: Max Rossmannek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woodsp-ibm woodsp-ibm woodsp-ibm left review comments

@pbark pbark pbark approved these changes

@mrossinek mrossinek mrossinek approved these changes

@manoelmarques manoelmarques Awaiting requested review from manoelmarques

@stefan-woerner stefan-woerner Awaiting requested review from stefan-woerner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ikkoham @woodsp-ibm @mrossinek @pbark