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

Hexagonal lattice followup #1232

Merged
merged 8 commits into from
Jun 28, 2024
Merged

Conversation

jpacold
Copy link
Contributor

@jpacold jpacold commented Jun 28, 2024

Follow-up to #1213.

  • Addressed comments on the previous PR

    • Split tests with a lot of sub-cases into subtests
    • Removed unnecessary module
  • We now pre-compute the number of edges in each graph, so that we can call G::with_capacity(self.num_nodes, self.num_edges) to create the graph where before we had G::with_capacity(self.num_nodes, self.num_nodes)

  • Fixed two bugs in the position calculation

    • In my initial implementation I missed a factor of two, so the hexagons were not actually regular
    • For lattices with an odd number of columns, the node index arithmetic had an error that led to configurations like this:
      hexlattice_bug
      Probably the reason I missed this is that I mostly wrote tests for cases with an even number of columns (required when periodic = True). We now get the expected result:
      hexlattice_fixed

@mtreinish
Copy link
Member

Could you add a release note to document the bugfixes? I'm planning to push a 0.15.1 release shortly and we can include this in it if you think it's warranted.

@coveralls
Copy link

coveralls commented Jun 28, 2024

Pull Request Test Coverage Report for Build 9712370219

Details

  • 130 of 131 (99.24%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.4%) to 95.781%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rustworkx-core/src/generators/hexagonal_lattice_graph.rs 128 129 99.22%
Files with Coverage Reduction New Missed Lines %
rustworkx-core/src/generators/hexagonal_lattice_graph.rs 1 98.4%
Totals Coverage Status
Change from base Build 9711339379: 0.4%
Covered Lines: 18048
Relevant Lines: 18843

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 28, 2024

Pull Request Test Coverage Report for Build 9712885224

Details

  • 130 of 131 (99.24%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.4%) to 95.781%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rustworkx-core/src/generators/hexagonal_lattice_graph.rs 128 129 99.22%
Files with Coverage Reduction New Missed Lines %
rustworkx-core/src/generators/hexagonal_lattice_graph.rs 1 98.4%
Totals Coverage Status
Change from base Build 9711339379: 0.4%
Covered Lines: 18048
Relevant Lines: 18843

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for catching and fixing this.

@coveralls
Copy link

coveralls commented Jun 28, 2024

Pull Request Test Coverage Report for Build 9713815577

Details

  • 130 of 131 (99.24%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.4%) to 95.781%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rustworkx-core/src/generators/hexagonal_lattice_graph.rs 128 129 99.22%
Files with Coverage Reduction New Missed Lines %
rustworkx-core/src/generators/hexagonal_lattice_graph.rs 1 98.4%
Totals Coverage Status
Change from base Build 9711339379: 0.4%
Covered Lines: 18048
Relevant Lines: 18843

💛 - Coveralls

@mtreinish mtreinish merged commit af5a6ba into Qiskit:main Jun 28, 2024
28 checks passed
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.

None yet

3 participants