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 HexagonalLattice Class #1027

Merged
merged 21 commits into from
Sep 4, 2023
Merged

Conversation

javabster
Copy link
Contributor

Summary

fixes #956

Details and comments

@javabster javabster marked this pull request as ready for review January 18, 2023 10:53
@coveralls
Copy link

coveralls commented Jan 18, 2023

Pull Request Test Coverage Report for Build 4053980683

  • 20 of 22 (90.91%) changed or added relevant lines in 2 files are covered.
  • 73 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.07%) to 85.963%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_nature/second_q/hamiltonians/lattices/hexagonal_lattice.py 19 21 90.48%
Files with Coverage Reduction New Missed Lines %
qiskit_nature/second_q/properties/electronic_density.py 2 98.39%
qiskit_nature/second_q/drivers/pyscfd/pyscfdriver.py 7 75.86%
qiskit_nature/second_q/mappers/qubit_converter.py 10 93.68%
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py 54 81.13%
Totals Coverage Status
Change from base Build 3901930559: 0.07%
Covered Lines: 17693
Relevant Lines: 20582

💛 - Coveralls

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.

Besides these comments, please also add a release note to this PR

@woodsp-ibm
Copy link
Member

Do we want to include this in the Lattice Models tutorial - since that tutorial shows the existing models it seems to me it would be good to include this there too.

@mrossinek mrossinek added this to the 0.7.0 milestone Jan 27, 2023
@mrossinek
Copy link
Member

Yesterday I found out that networkx supports both: periodic boundary conditions and the computation of the real-space coordinates of a regular hexagonal lattice structure (something which we like to use to make the graphs look predictable; see for example here).

@mtreinish is rustworkx open to supporting similar functionality? At least periodic boundary conditions would seem useful to me.

If yes, I suggest we could track the addition of similar functionality there. Since I moved this PR to 0.7.0 we could even postpone until that is available. @javabster wold you be interested in contributing this to rustworkx, assuming they want it 😄

@mtreinish
Copy link
Collaborator

Yeah, both of those seem totally valid to add to rustworkx. If you don't mind can you open an enhancement issue:

https://github.com/Qiskit/rustworkx/issues/new?assignees=&labels=type%3A+enhancement&template=ENHANCEMENT_REQUEST.md

with the details and we can target it for the 0.13.0 release (which we don't have a target date for yet). @javabster it'd be great if you're interested in contributing the functionality to rustworkx too. I can give you a hand if you need any help getting started on it.

@javabster
Copy link
Contributor Author

issue opened: Qiskit/rustworkx#803

@mrossinek
Copy link
Member

For various bandwidth reasons I would like to restrict the scope of this PR as follows:

  • we neglect periodicity for the time being and instead add support for this in the future (separate issue to be opened)
  • we need to add a computation of the default node positions (to ensure consistent and nice visualization; see for example the networkx implementation of this)
  • in the future, rustworkx might do this last point directly at which point we can remove this from this class. Thus, this computation should be done in a private method which we can remove without API breakage

@CLAassistant
Copy link

CLAassistant commented Jun 7, 2023

CLA assistant check
All committers have signed the CLA.

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.

Let's also get this one merged. I added a comment here to keep track of possible future extensions.

I also added a simple release note in a similar style to the one of the recently merged Kagome lattice.

Thank you, Abby, for contributing to Qiskit Nature and being so patient with this lengthy (and sometimes stalled) review process!

@mergify mergify bot merged commit 9b48d54 into qiskit-community:main Sep 4, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a HexagonalLattice
6 participants