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 PolynomialTensor class #756

Merged
merged 36 commits into from
Sep 14, 2022
Merged

Conversation

SaashaJoshi
Copy link
Contributor

@SaashaJoshi SaashaJoshi commented Jul 20, 2022

Summary

Adding a [draft] file for PolynomialTensor class.
[WIP] corresponding unittest.
Closes #665

Details and comments

The PolynomialTensor class overrides:

  1. _mul and _add functions from StarAlgebraMixin LinearMixin
  2. conjugate and transpose methods from AdjointMixin

More brainstorming needs to be done for compose and adjoint functions.

@CLAassistant
Copy link

CLAassistant commented Jul 20, 2022

CLA assistant check
All committers have signed the CLA.

@SaashaJoshi SaashaJoshi marked this pull request as draft July 20, 2022 14:11
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.

Some suggestions from my side. Other than that, this looks to be shaping up quite nicely 👍

qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Aug 12, 2022

Pull Request Test Coverage Report for Build 3052969424

  • 65 of 80 (81.25%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 85.299%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_nature/second_q/operators/polynomial_tensor.py 64 79 81.01%
Totals Coverage Status
Change from base Build 3051660763: -0.02%
Covered Lines: 16809
Relevant Lines: 19706

💛 - Coveralls

@SaashaJoshi SaashaJoshi requested review from woodsp-ibm and removed request for woodsp-ibm August 23, 2022 16:43
Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>
@mrossinek
Copy link
Member

I had another discussion with @woodsp-ibm and we said that it might be beneficial to directly implement PolynomialTensor as a subclass of Mapping. That would give full flexibility when accessing the internal _data object.
The way that you could achieve this, is by something similar to the following:

from collections.abc import Mapping
from typing import Iterator

class PolynomialTensor(..., Mapping):
    ...

    def __getitem__(self, __k: str) -> np.ndarray | Number:
        return self._data.__getitem__(__k)

    def __len__(self) -> int:
        return self._data.__len__()

    def __iter__(self) -> Iterator[str]:
        return self._data.__iter__()

This will directly enable users to use len(...), .keys(), .values(), .items() and even use p_t["key"] to access specific entries 👍

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.

Please also double check the formatting of the docstrings. Especially PolynomialTensor should always be written as a single word

qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
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.

Just some minor comments. Other than this, I think this is almost good to go 👍

qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
qiskit_nature/second_q/operators/polynomial_tensor.py Outdated Show resolved Hide resolved
@SaashaJoshi SaashaJoshi requested review from mrossinek and woodsp-ibm and removed request for mrossinek September 13, 2022 20:08
@SaashaJoshi SaashaJoshi marked this pull request as ready for review September 13, 2022 20:09
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'm happy with this as is! Thanks a lot, Saasha, for your continuous effort on this! 🙂

I'm sure that there will be some minor changes that I will be doing to this as I work on #666 but overall this looks like it is in very good shape to be merged now 👍



class PolynomialTensor(LinearMixin, AdjointMixin, TolerancesMixin, Mapping):
"""PolynomialTensor class"""
Copy link
Member

Choose a reason for hiding this comment

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

For a future reviewer: I will flesh out this docstring in an upcoming PR when working on #666 👍

@woodsp-ibm woodsp-ibm changed the title adding PolynomialTensor class Add PolynomialTensor class Sep 14, 2022
@mrossinek mrossinek merged commit d1b5e07 into qiskit-community:main Sep 14, 2022
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request May 25, 2023
* adding PolynomialTensor class

* adding unittest corresponding PolynomialTensor class; making changes to the class as suggested

* adding conjugate and transpose functions

* made changes to polynomial_tensor and test_polyomial_tensor files

* Update qiskit_nature/second_q/operators/polynomial_tensor.py

Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>

* updating polynomial_tensor and unittest

* updating polynomial tensor and unittest

* update unittest

* updating polynomial_tensor file

* Update qiskit_nature/second_q/operators/polynomial_tensor.py

Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>

* Update qiskit_nature/second_q/operators/polynomial_tensor.py

Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>

* Update qiskit_nature/second_q/operators/polynomial_tensor.py

Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>

* Update qiskit_nature/second_q/operators/polynomial_tensor.py

Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>

* update polynomial_tensor and test_polynomial_tensor

* adding register_length to polynomial tensor

* Update qiskit_nature/second_q/operators/polynomial_tensor.py

Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>

* changes to polynomial_tensor - adding register_length

* adding equiv method to polynomial_tensor. adding unittests for dunders

* lint and mypy update

* Update qiskit_nature/second_q/operators/polynomial_tensor.py

Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>

* updating polynomial_tensor

* Update qiskit_nature/second_q/operators/polynomial_tensor.py

Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>

* update polynomial_tensor

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a PolynomialTensor class
6 participants