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

Fix issue1258 #1259

Merged
merged 6 commits into from
Sep 27, 2023
Merged

Fix issue1258 #1259

merged 6 commits into from
Sep 27, 2023

Conversation

grossardt
Copy link
Contributor

@grossardt grossardt commented Sep 24, 2023 •
edited
Loading

Summary

Fixes issue #1258

For BosonicOp, FermionicOp, SpinOp, VibrationalOp, changed the line that takes care of the empty string coefficient (constant part of operator) in from_polynomial_tensor constructor to assign a scalar number rather than 0d array.

Details and comments

PolynomialTensor stores all coefficients as tensors, including the empty string contribution, which is consistent. The SparseLabelOps have a scalar number coefficient per label string. When constructed from PolynomialTensor, coefficients are assigned from the scalar elements of the corresponding tensor. However, the constant term (empty string) is dealt with separately, assigning the 0d tensor:

data[""] = tensor[key]

To convert this also to scalar, this is replaced by:

data[""] = tensor[key].sum()

@CLAassistant
Copy link

CLAassistant commented Sep 24, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coveralls
Copy link

coveralls commented Sep 24, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 6315372064

  • 1 of 4 (25.0%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 86.73%
Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_nature/second_q/operators/bosonic_op.py 0 1 0.0%
qiskit_nature/second_q/operators/spin_op.py 0 1 0.0%
qiskit_nature/second_q/operators/vibrational_op.py 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
qiskit_nature/second_q/operators/tensor.py 1 85.46%
Totals Coverage Status
Change from base Build 6195281139: -0.01%
Covered Lines: 8706
Relevant Lines: 10038
💛 - Coveralls

mrossinek
mrossinek reviewed Sep 25, 2023
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.

Interesting approach, thanks for fixing this!

Before merging, please also add a release note which explains the bug and states that it has been fixed. Please focus on this being an end-user centric document, so try to keep it from being too technical.

@mrossinek mrossinek linked an issue Sep 25, 2023 that may be closed by this pull request
from_polynomial_tensor constructor creates 0d array instead of scalar for constant coefficient #1258
Closed
@grossardt
Copy link
Contributor Author

Sorry, I misinterpreted the CONTRIBUTING.md, assuming that fixes like this don't count as 'end user facing' and therefore don't need release notes. I added a short release note now, I hope this is appropriate in length and detail.

@woodsp-ibm
Copy link
Member

Maybe this might logically be better to convert to scalar https://numpy.org/doc/stable/reference/generated/numpy.ndarray.item.html ? While sum works you did have to comment to explain what its really doing.

@grossardt
Copy link
Contributor Author

@woodsp-ibm
Thanks. I changed it accordingly. Tried to keep it compact without messing around with dtype and still get a scalar of the right type, but I simply didn't know of the existence of ndarray.item(), thats why .sum() seemed most fitting.

mrossinek
mrossinek reviewed Sep 26, 2023
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.

I prefer the .item() method for converting to scalar. I actually did not know about this, so thanks a lot, Steve, for pointing this out! 👍

mrossinek
mrossinek approved these changes Sep 27, 2023
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.

Thank you for your contribution! 👍

@mrossinek mrossinek merged commit 60cac2f into qiskit-community:main Sep 27, 2023
@grossardt grossardt deleted the fix-issue1258 branch September 27, 2023 07:54
ialsina pushed a commit to ialsina/qiskit-nature that referenced this pull request Nov 17, 2023
@grossardt @mrossinek @ialsina
Fix issue1258 (qiskit-community#1259)
2cf80d5 
* fixes issue 1258

* style fixes after make black

* Add release note for fix-issue1258

* replace sum() with item() to get scalar

* Update releasenotes/notes/fix-from-polynomial-tensor-constant-9df7f63be4a4c819.yaml

Co-authored-by: Max Rossmannek <[email protected]>

* Update fix-from-polynomial-tensor-constant-9df7f63be4a4c819.yaml

---------

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

@mrossinek mrossinek mrossinek approved these changes

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

@woodsp-ibm woodsp-ibm Awaiting requested review from woodsp-ibm woodsp-ibm is a code owner

@ftroisi ftroisi Awaiting requested review from ftroisi ftroisi is a code owner

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

Successfully merging this pull request may close these issues.

from_polynomial_tensor constructor creates 0d array instead of scalar for constant coefficient
5 participants
@grossardt @CLAassistant @coveralls @woodsp-ibm @mrossinek