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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix several issues in find_hamiltonian #35956

Merged
merged 4 commits into from Jul 30, 2023
Merged

Fix several issues in find_hamiltonian #35956

merged 4 commits into from Jul 30, 2023

Conversation

dcoudert
Copy link
Contributor

Part of #35902.

The method find_hamiltonian had multiple issues: it was not robust to vertices with incomparable labels, it could loop forever if a vertex has degree 1, a segfault was possible on small graphs, it was not properly working for digraphs, etc.
We fix multiple issues and the method should now be safe.

馃摑 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

@dcoudert dcoudert requested a review from jhpalmieri July 14, 2023 13:27
@dcoudert dcoudert self-assigned this Jul 14, 2023
@dcoudert dcoudert mentioned this pull request Jul 14, 2023
Open
1 task
@dcoudert dcoudert requested a review from dimpase July 15, 2023 08:21
dimpase
dimpase approved these changes Jul 15, 2023
View reviewed changes
Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

lgtm

@dcoudert
Copy link
Contributor Author

Thank you Dima. Can I set to positive review ?

@dimpase
Copy link
Member

dimpase commented Jul 15, 2023

yes, please

@dcoudert
Copy link
Contributor Author

Thank you.

@vbraun
Copy link
Member

vbraun commented Jul 19, 2023

On Ubuntu 18.04 64 bit and 20.04 64 bit:

**********************************************************************
File "src/sage/graphs/generic_graph_pyx.pyx", line 1256, in sage.graphs.generic_graph_pyx.?
Failed example:
    fh(G)
Expected:
    (False, ['a', 'b', 'c', 'd'])
Got:
    (False, ['x', 'y', 'd'])
**********************************************************************

dcoudert added a commit to dcoudert/sage that referenced this pull request Jul 20, 2023
@dcoudert
PR sagemath#35956: fix doctests
842170e
dcoudert added a commit to dcoudert/sage that referenced this pull request Jul 20, 2023
@dcoudert
PR sagemath#35956: fix typo
b8b98f1
@dcoudert
Copy link
Contributor Author

I changed the doctest to something more robust for this random heuristic.

@github-actions
Copy link

Documentation preview for this PR (built with commit fb35dc4; changes) is ready! 馃帀

@dimpase dimpase self-requested a review July 26, 2023 18:15
dimpase
dimpase approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

OK

@dcoudert
Copy link
Contributor Author

Thank you Dima. I hope it's ok now.

@vbraun vbraun merged commit ce5b249 into sagemath:develop Jul 30, 2023
12 of 13 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 30, 2023
dkrenn added a commit to dkrenn/sage that referenced this pull request Jul 31, 2023
@dkrenn
Merge remote-tracking branch 'upstream/develop' into HEAD
26ca939 
* upstream/develop: (1372 commits)
  Updated SageMath version to 10.1.beta8
  remove multiple call of Vmatrix and Vmodule
  remove PGE and some listcomp
  get two entry directly
  tests passed
  Still permutation
  wip fix one issue
  fix
  fix typo
  store inverse of permutation
  Direct_Permute
  WIP PermutedMatrixWindow
  redundant line
  Style fixes
  PR sagemath#35891: fix issue in src/sage/geometry/polyhedral_complex.py
  fix merging problems
  add type checks for parameters
  PR sagemath#35956: fix typo
  PR sagemath#35956: fix doctests
  fix another issue
  ...
dkrenn added a commit to dkrenn/sage that referenced this pull request Aug 3, 2023
@dkrenn
Merge tag '10.1.beta8' into k-regular-bounded
5e57125 
SageMath version 10.1.beta8, Release Date: 2023-07-30

* tag '10.1.beta8': (13890 commits)
  Updated SageMath version to 10.1.beta8
  remove multiple call of Vmatrix and Vmodule
  remove PGE and some listcomp
  get two entry directly
  tests passed
  Still permutation
  wip fix one issue
  fix
  fix typo
  store inverse of permutation
  Direct_Permute
  WIP PermutedMatrixWindow
  redundant line
  Style fixes
  PR sagemath#35891: fix issue in src/sage/geometry/polyhedral_complex.py
  fix merging problems
  add type checks for parameters
  PR sagemath#35956: fix typo
  PR sagemath#35956: fix doctests
  fix another issue
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimpase dimpase dimpase approved these changes

@jhpalmieri jhpalmieri Awaiting requested review from jhpalmieri

Assignees

@dcoudert dcoudert

Labels
c: graph theory p: major / 3
Projects
None yet
Milestone
sage-10.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dcoudert @dimpase @vbraun @mkoeppe