-
-
Notifications
You must be signed in to change notification settings - Fork 523
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 method to compute the longest (induced) cycle in a (di)graph #37028
Conversation
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.
Some doc nitpicks.
I am also guessing you cannot say anything about how the solution improves for setting h
in line 8141. In particular, it makes sense to build a new graph each iteration rather than manipulate a single (mutable) graph?
Indeed, the set of selected edges can be very different from one iteration to the next. Furthermore, the time needed to build the graph is expected to be small compared to the time needed to solve the ILP. |
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. That is what I was thinking. Positive review.
Thank you for the review. |
Documentation preview for this PR (built with commit 8b71477; changes) is ready! 🎉 |
…n a (di)graph This PR adds a method to compute the longest (induced) cycle in a (di)graph. The method can also consider weighted cases. This answers a request from https://ask.sagemath.org/question/75124/how- to-find-a-longest-cycle-and-a-longest-induced-cycle-in-a-graph/ ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37028 Reported by: David Coudert Reviewer(s): David Coudert, Travis Scrimshaw
…n a (di)graph This PR adds a method to compute the longest (induced) cycle in a (di)graph. The method can also consider weighted cases. This answers a request from https://ask.sagemath.org/question/75124/how- to-find-a-longest-cycle-and-a-longest-induced-cycle-in-a-graph/ ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37028 Reported by: David Coudert Reviewer(s): David Coudert, Travis Scrimshaw
…n a (di)graph This PR adds a method to compute the longest (induced) cycle in a (di)graph. The method can also consider weighted cases. This answers a request from https://ask.sagemath.org/question/75124/how- to-find-a-longest-cycle-and-a-longest-induced-cycle-in-a-graph/ ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37028 Reported by: David Coudert Reviewer(s): David Coudert, Travis Scrimshaw
Hi, can you try this example?
Currently, my SageMath version is 10.2, and I excerpted your code as follows, but it seems that the longest cycle and the longest induced cycle are both having issues for the graph.
It shows a longest cycle of Z is of length 5 and a longest induced cycle is of length 4. But we can find a cycle of Z of length 9 and a longest induced cycle of length 5. Did I miss something in my excerpt? |
Right, the subtour elimination constraints are not correct for the longest cycle. I proposed another formulation in #37181. |
An issue has been raised (see sagemath#37028 (comment)) on the formulation used to find the longest (induced) cycle. This was due to the subtour elimination constraints that were not correct. We change these constraints to fix this issue. The new constraints force to use edges from the boundary of a subtour only when a vertex of that subtour is selected. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37181 Reported by: David Coudert Reviewer(s): Travis Scrimshaw
An issue has been raised (see sagemath#37028 (comment)) on the formulation used to find the longest (induced) cycle. This was due to the subtour elimination constraints that were not correct. We change these constraints to fix this issue. The new constraints force to use edges from the boundary of a subtour only when a vertex of that subtour is selected. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37181 Reported by: David Coudert Reviewer(s): Travis Scrimshaw
An issue has been raised (see sagemath#37028 (comment)) on the formulation used to find the longest (induced) cycle. This was due to the subtour elimination constraints that were not correct. We change these constraints to fix this issue. The new constraints force to use edges from the boundary of a subtour only when a vertex of that subtour is selected. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37181 Reported by: David Coudert Reviewer(s): Travis Scrimshaw
This PR adds a method to compute the longest (induced) cycle in a (di)graph. The method can also consider weighted cases.
This answers a request from https://ask.sagemath.org/question/75124/how-to-find-a-longest-cycle-and-a-longest-induced-cycle-in-a-graph/
📝 Checklist
⌛ Dependencies