Bosonic operators #1085

ftroisi · 2023-03-03T12:30:44Z

Summary

Details and comments

…d results

CLAassistant · 2023-03-03T12:30:49Z

All committers have signed the CLA.

mrossinek

A few initial observations. We will also need to add a release note 👍

qiskit_nature/second_q/operators/bosonic_op.py

…ation. Fixed tests

mrossinek

Overall I think this PR is already in pretty good shape. I left quite a few comments, some of which
are nitpicks, others are a bit more broad. In general, I'd like to add these notes:

we need to add a release note (see here)
you need to run the code formatter: make black (from the repo root)
you need to add bosonicop as a spell exception to .pylintdict
the docstrings are mostly not formatted properly. This can be quite tricky when just getting
started so I suggest this in our offline-scheduled meeting. I left some pointers in my
comments below already though.

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py

qiskit_nature/second_q/operators/bosonic_op.py

mrossinek · 2023-05-02T13:35:02Z

qiskit_nature/second_q/operators/bosonic_op.py

+        ordered_op = self.normal_order()
+
+        data = defaultdict(complex)  # type: dict[str, _TCoeff]
+        # TODO: use parallel_map to make this more efficient (?)


FYI for another reviewer: this TODO is copied over from FermionicOp and out-of-scope for this PR

test/second_q/mappers/test_bosonic_linear_mapper.py

qiskit_nature/second_q/operators/bosonic_op.py

ElePT

Here are a few more comments in preparation for our meeting (no need to address them before).

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py

qiskit_nature/second_q/operators/bosonic_op.py

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py

qiskit_nature/second_q/operators/bosonic_op.py

Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Max Rossmannek <[email protected]>

ElePT

Thanks Francesco! Just a few more steps. We were looking for a specific way of defining properties and property setters, see specific comments in the code.

qiskit_nature/second_q/mappers/bosonic_mapper.py

Co-authored-by: Elena Peña Tapia <[email protected]>

ElePT

Yay! The CI passes!! I took a look at the rendered docs and I think there are two last things that can be improved (getting picky here):

The definitions of the sigma +/- operators should be in separate lines (idk why Sphinx is not getting the line break)
I think that the two references would look better if they were in separate lines too.

I added two suggestions that will hopefully fix this :)

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py

ElePT

One last last thing

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py

Co-authored-by: Elena Peña Tapia <[email protected]>

ElePT

Sorry, there was a missing empty line in my code block suggestion from before and it didn't render properly. New suggestion should fix that (also added an empty line between imports and code in the example below because I think it looks a bit better)

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py

Co-authored-by: Elena Peña Tapia <[email protected]>

ElePT

Max will probably give a last round of review, but in terms of docs, I have nothing to say other than good job. Thanks a lot!

mrossinek

Okay, some last final nitpicky comments.
Once these changes are applied I'm confident we can merge this PR 👍

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py

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

qiskit_nature/second_q/mappers/bosonic_mapper.py

mrossinek

Assuming I did not mess up my prior suggestions, CI should pass on this...

Thanks a lot, Francesco, for your hard work on this great contribution to Qiskit Nature! 🎉
Thank you, Elena, for your support in reviewing and helping Francesco out on the home stretch of this PR.

qiskit_nature/second_q/mappers/bosonic_mapper.py

* Created bosonic operator file. Some notes on the code * modified ordering functions using commutator instead of anti-commutator * Created simplify method * Created test class for bosonic operator * Moved normal_order and index_order before simplify. Fixed the expected results * Fixed tests. Hermicity test is still missing * Improved algorithm for simplify. Now calls normal order as first operation. Fixed tests * fixed is_hermitian method * Addressed PR comments * Pipelines fixes * Created bosonic_linear_mapper class * Added some unit tests for bosonic linear mapper. Pipelines fixes * Removed trailing whitspace * Fixed test case +_0 -_0 with truncation 2 * Implemented test cases +_0 -_1 and -_1 +_0. Some CI fixes * Added test case +_0 +_0 * Some CI fixes * Apply suggestions from code review Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Max Rossmannek <[email protected]> * Added test for commutator relation * Added terms to pylintdict * Updated some mapper tests * Addressed some PR comments. Fixed final tests * Addressed PR comments for docstrings * Further doc fixes * Further CI fixes * Added release note * Spell fix in release note * Added methods from_terms and _permute_terms * Addressed PR comments * ran make black * Added r in front of docstring * Added reference for bosonic op truncation * Addressed PR comments * Added mapper example in bosonicLinearMapper docString * Updated error message * Renamed truncation property. Defined its setter * Apply suggestions for max_occupation setter Co-authored-by: Elena Peña Tapia <[email protected]> * ran make black * Removed unused import * Doc fixes Co-authored-by: Elena Peña Tapia <[email protected]> * Doc fix Co-authored-by: Elena Peña Tapia <[email protected]> * ran make black * Doc fix Co-authored-by: Elena Peña Tapia <[email protected]> * ran make black * Apply suggestions from code review Co-authored-by: Max Rossmannek <[email protected]> * Minor doc fix * Apply suggestions from code review * Update qiskit_nature/second_q/mappers/bosonic_mapper.py --------- Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Max Rossmannek <[email protected]>

ftroisi added 5 commits March 2, 2023 10:22

Created bosonic operator file. Some notes on the code

358c7b9

modified ordering functions using commutator instead of anti-commutator

5b169f9

Created simplify method

ccc882f

Created test class for bosonic operator

7eaae1f

Moved normal_order and index_order before simplify. Fixed the expecte…

a6b6374

…d results

Fixed tests. Hermicity test is still missing

4f42c8d

mrossinek reviewed Mar 5, 2023

View reviewed changes

ftroisi added 5 commits March 7, 2023 10:57

Improved algorithm for simplify. Now calls normal order as first oper…

07bcdd1

…ation. Fixed tests

fixed is_hermitian method

1fb0269

Addressed PR comments

1963209

Pipelines fixes

7efafc8

Created bosonic_linear_mapper class

5314730

mrossinek added this to the 0.7.0 milestone Mar 10, 2023

Added some unit tests for bosonic linear mapper. Pipelines fixes

04b9cc9

mrossinek mentioned this pull request Apr 5, 2023

Implement Mappers for d-level Operators #1101

Open

ftroisi added 5 commits April 27, 2023 10:38

Removed trailing whitspace

ca76367

Fixed test case +_0 -_0 with truncation 2

2fb1514

Implemented test cases +_0 -_1 and -_1 +_0. Some CI fixes

1dcae8a

Added test case +_0 +_0

ef07ee6

Some CI fixes

78911b3

mrossinek reviewed May 2, 2023

View reviewed changes

mrossinek reviewed May 3, 2023

View reviewed changes

qiskit_nature/second_q/operators/bosonic_op.py Outdated Show resolved Hide resolved

mrossinek requested a review from ElePT May 3, 2023 14:25

ElePT reviewed May 15, 2023

View reviewed changes

ftroisi and others added 5 commits May 15, 2023 17:36

Apply suggestions from code review

1556c21

Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Max Rossmannek <[email protected]>

Added test for commutator relation

c95e383

Added terms to pylintdict

071e6af

Updated some mapper tests

e975e5d

Addressed some PR comments. Fixed final tests

0d8cdaa

ftroisi added 2 commits May 21, 2023 13:49

Updated error message

670b44a

Renamed truncation property. Defined its setter

fa53e96

ElePT reviewed May 22, 2023

View reviewed changes

qiskit_nature/second_q/mappers/bosonic_mapper.py Outdated Show resolved Hide resolved

qiskit_nature/second_q/mappers/bosonic_mapper.py Outdated Show resolved Hide resolved

qiskit_nature/second_q/mappers/bosonic_mapper.py Outdated Show resolved Hide resolved

ftroisi and others added 3 commits May 22, 2023 11:27

Apply suggestions for max_occupation setter

c45fcf7

Co-authored-by: Elena Peña Tapia <[email protected]>

ran make black

71ae4fa

Removed unused import

1254ac0

ElePT reviewed May 22, 2023

View reviewed changes

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py Show resolved Hide resolved

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py Outdated Show resolved Hide resolved

ElePT reviewed May 22, 2023

View reviewed changes

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py Outdated Show resolved Hide resolved

ftroisi and others added 3 commits May 22, 2023 15:04

Doc fixes

7e0b58c

Co-authored-by: Elena Peña Tapia <[email protected]>

Doc fix

8fb988e

Co-authored-by: Elena Peña Tapia <[email protected]>

ran make black

1cf574c

ElePT reviewed May 22, 2023

View reviewed changes

qiskit_nature/second_q/mappers/bosonic_linear_mapper.py Outdated Show resolved Hide resolved

ftroisi and others added 2 commits May 22, 2023 16:16

Doc fix

f7f9b3c

Co-authored-by: Elena Peña Tapia <[email protected]>

ran make black

80af128

ElePT previously approved these changes May 22, 2023

View reviewed changes

mrossinek reviewed May 22, 2023

View reviewed changes

Apply suggestions from code review

eeb1c1a

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

ftroisi dismissed ElePT’s stale review via eeb1c1a May 22, 2023 15:52

Minor doc fix

8a1179c

woodsp-ibm mentioned this pull request May 22, 2023

Improve docs #1175

Merged

mrossinek reviewed May 23, 2023

View reviewed changes

qiskit_nature/second_q/mappers/bosonic_mapper.py Outdated Show resolved Hide resolved

qiskit_nature/second_q/mappers/bosonic_mapper.py Outdated Show resolved Hide resolved

Apply suggestions from code review

d246aab

mrossinek previously approved these changes May 23, 2023

View reviewed changes

mrossinek reviewed May 23, 2023

View reviewed changes

qiskit_nature/second_q/mappers/bosonic_mapper.py Outdated Show resolved Hide resolved

Update qiskit_nature/second_q/mappers/bosonic_mapper.py

11fe4f6

mrossinek dismissed their stale review via 11fe4f6 May 23, 2023 06:51

mrossinek approved these changes May 23, 2023

View reviewed changes

mrossinek added the automerge label May 23, 2023

mergify bot merged commit f67eb91 into qiskit-community:main May 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bosonic operators #1085

Bosonic operators #1085

ftroisi commented Mar 3, 2023

CLAassistant commented Mar 3, 2023 •

edited

Loading

mrossinek left a comment

mrossinek left a comment •

edited

Loading

mrossinek May 2, 2023

ElePT left a comment

ElePT left a comment

ElePT left a comment

ElePT left a comment

ElePT left a comment •

edited

Loading

ElePT left a comment

mrossinek left a comment

mrossinek left a comment

Bosonic operators #1085

Bosonic operators #1085

Conversation

ftroisi commented Mar 3, 2023

Summary

Details and comments

CLAassistant commented Mar 3, 2023 • edited Loading

mrossinek left a comment

Choose a reason for hiding this comment

mrossinek left a comment • edited Loading

Choose a reason for hiding this comment

mrossinek May 2, 2023

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

ElePT left a comment • edited Loading

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

mrossinek left a comment

Choose a reason for hiding this comment

mrossinek left a comment

Choose a reason for hiding this comment

CLAassistant commented Mar 3, 2023 •

edited

Loading

mrossinek left a comment •

edited

Loading

ElePT left a comment •

edited

Loading