-
Notifications
You must be signed in to change notification settings - Fork 210
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
Adds documentation for the default_filter_criterion #726
Adds documentation for the default_filter_criterion #726
Conversation
…inglet spin configuration assumption.
qiskit_nature/problems/second_quantization/electronic/electronic_structure_problem.py
Outdated
Show resolved
Hide resolved
qiskit_nature/problems/second_quantization/electronic/electronic_structure_problem.py
Outdated
Show resolved
Hide resolved
qiskit_nature/problems/second_quantization/electronic/electronic_structure_problem.py
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 2801582482
💛 - Coveralls |
qiskit_nature/problems/second_quantization/electronic/electronic_structure_problem.py
Outdated
Show resolved
Hide resolved
test/algorithms/excited_state_solvers/test_excited_states_solvers.py
Outdated
Show resolved
Hide resolved
…he class docstring. Changes the test for custom filters to a more relevant example with the doublets of BeH
…y-Gandon/qiskit-nature into document_filter_criterion
We will have to ensure this gets this into the relocated classes too from the refactor of #722 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As previously mentioned by Steve, we have restructured the code base, meaning that this change now needs to be applied to the class located at qiskit_nature/second_q/problems
and needs to leave the old code unchanged. A matching test location exists, too.
Other than that, this PR looks good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Anthony-Gandon!
qiskit_nature/second_q/problems/electronic_structure_problem.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than this, this LGTM 👍
qiskit_nature/second_q/problems/electronic_structure_problem.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Anthony!
…#726) * Adds documentation for the default_filter_criterion to explicit the singlet spin configuration assumption. * Fixes the documentation. * Fixes some checks. * Fixes style. * Moves the docstring from the method get_default_filter_criterion to the class docstring. Changes the test for custom filters to a more relevant example with the doublets of BeH * Changes are now applied to the class located at qiskit_nature/second_q/problems * Fixes the test * Fixes the test * Fixes the test * Updates documentation the test * Retests after coverall issue * Retest after coverall issue * Fix indenting of the docstring * Update qiskit_nature/second_q/problems/electronic_structure_problem.py Co-authored-by: Max Rossmannek <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Max Rossmannek <[email protected]>
Fix #643
Summary
Adds a note in the docstring of the default_filter_criterion() method to explicit that it is assuming singlet spin configuration.
An exemple of how a custom filter could be given for other configurations is also provided (it is only an attempt because I don't yet know how to ensure that the AngularMomentum property exists)
Details and comments
At the moment and with a simple setup for the problem, I'm getting
None
when trying to access the AngularMomentum property.How could I setup a working test for this?