Fix reporting weight in yen_k_shortest_simple_paths #40248
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcoudert
added
s: needs review
c: graph theory
gsoc: 2025
dcoudert
suggested changes
Jun 12, 2025
|
|
||
| sage: g = DiGraph([(1, 2)]) | ||
| sage: list(yen_k_shortest_simple_paths(g, 1, 1, report_edges=True, report_weight=True)) | ||
| [(0, [])] | ||
| """ | ||
| if source not in self: | ||
| raise ValueError("vertex '{}' is not in the graph".format(source)) | ||
| if target not in self: | ||
| raise ValueError("vertex '{}' is not in the graph".format(target)) | ||
|
|
||
| if source == target: |
There was a problem hiding this comment.
A shorter version could be
if source == target:
P = [] if report_edges else [source]
yield (0, P) if report_weight else P
src/sage/graphs/path_enumeration.pyx
Outdated
| @@ -734,19 +734,29 @@ def yen_k_shortest_simple_paths(self, source, target, weight_function=None, | |||
| [1, 6, 9, 10, 5], | |||
| [1, 6, 9, 3, 4, 5], | |||
| [1, 6, 9, 11, 10, 5]] | |||
|
|
|||
| When s = t and report_edge=True and report_weight=True (issue 40247):: | |||
There was a problem hiding this comment.
Correct form, in particular to link the issue:
When ``s == t`` and ``report_edge == True`` and ``report_weight == True`` (:issue:`40247`)::
|
Documentation preview for this PR (built with commit 77a304f; changes) is ready! 🎉 |
|
also, make sure the PR is not in conflict with #40217 |
|
There is something wrong because now this PR and #40217 have exactly the same code. I think one of the PR (this one) should only modify Yen. Then #40217 should be (re)built on top of this one, and so depend on this one. The alternative is to do everything in the same PR, but with the merge commit you have done, I'm afraid you will have to force push a fresh branch with all modifications. |
|
Its my mistake. I understand the second plan and it may be better. But I cannot understand why I need to force push a fresh branch with all modifications because I don't understand git...... Can I use the current branch in creating new PR? |
|
may be @dimpase can give a better advise than me. I'm not a git guru and I don't know what may happen if a branch is based on commits from an abandoned branch/PR. |
|
a PR can of course be based on any branch, but to be merged it needs to be mergeable with the current development branch, or, even better, based on the latter. The need for force-pushing is not unusual. After a rebase you almost always need to force-push to the remote. What exactly are you trying to do? |
|
there is absolutely no need for abandoning a PR and starting a new one to solve the same problem. (you might want to keep an old branch in your fork under a different name, this is easy, I can tell you how to do this) |
|
My solution now is in both report_weight_in_yen and enum_cycle_undirected branches, reset --hard as follows (go back to the last commit for each branch before merge). sage git:(report_weight_in_yen) git reset --hard f2ef3533ea9sage git:(enum_cycle_undirected) git reset --hard 9057ffd3933and only merge report_weight_in_yen into enum_cycle_undirected. sage git:(enum_cycle_undirected) git merge report_weight_in_yen and force push both branches. By doing so,
I think there will be no conflict in these PRs. Is it ok? (in particular, this changes the current commit history) |
kappybar
force-pushed
the
report_weight_in_yen
branch
from
4b4a3e3 to
f2ef353
Compare
|
I have finished the above changes. |
|
FYI, reported lint errors are independent from this PR and fixed in #40254. |
5 tasks
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jun 17, 2025
sagemathgh-40217: Enum cycle in an undirected graph (and fix bug in yen_k_shortest_simple_path algorithm) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> Add implementation of enumeration of cycles in an undirected graph. Specifically, I make ```sage/graphs/cycle_enumeration.py``` and modify + ```~sage.graphs.cycle_enumeration._all_simple_cycles_iterator_edge``` + ```~sage.graphs.cycle_enumeration.all_cycles_iterator``` + ```~sage.graphs.cycle_enumeration.all_simple_cycles``` In implementing these funcionts, I fix bug in ```~sage.graphs.path_enumeration.yen_k_shortest_simple_paths```. This bug occurs when there is no path between source and target. Also, I add ```~sage.graphs.connectivity.biconnected_components_subgraphs```. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [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 and checked the documentation preview. ### ⌛ Dependencies sagemath#40248 <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40217 Reported by: Yuta Inoue Reviewer(s): David Coudert
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jun 17, 2025
sagemathgh-40248: Fix reporting weight in yen_k_shortest_simple_paths <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> If source equals target, and report_edge=True, report_weight=True, then weight is not reported in yen_k_shortest_simple_paths. This PR fixes this issue. Fix sagemath#40247. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [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 and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40248 Reported by: Yuta Inoue Reviewer(s): David Coudert
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jun 18, 2025
sagemathgh-40217: Enum cycle in an undirected graph (and fix bug in yen_k_shortest_simple_path algorithm) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> Add implementation of enumeration of cycles in an undirected graph. Specifically, I make ```sage/graphs/cycle_enumeration.py``` and modify + ```~sage.graphs.cycle_enumeration._all_simple_cycles_iterator_edge``` + ```~sage.graphs.cycle_enumeration.all_cycles_iterator``` + ```~sage.graphs.cycle_enumeration.all_simple_cycles``` In implementing these funcionts, I fix bug in ```~sage.graphs.path_enumeration.yen_k_shortest_simple_paths```. This bug occurs when there is no path between source and target. Also, I add ```~sage.graphs.connectivity.biconnected_components_subgraphs```. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [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 and checked the documentation preview. ### ⌛ Dependencies sagemath#40248 <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40217 Reported by: Yuta Inoue Reviewer(s): David Coudert
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jun 18, 2025
sagemathgh-40248: Fix reporting weight in yen_k_shortest_simple_paths <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> If source equals target, and report_edge=True, report_weight=True, then weight is not reported in yen_k_shortest_simple_paths. This PR fixes this issue. Fix sagemath#40247. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [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 and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40248 Reported by: Yuta Inoue Reviewer(s): David Coudert
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jun 21, 2025
sagemathgh-40217: Enum cycle in an undirected graph (and fix bug in yen_k_shortest_simple_path algorithm) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> Add implementation of enumeration of cycles in an undirected graph. Specifically, I make ```sage/graphs/cycle_enumeration.py``` and modify + ```~sage.graphs.cycle_enumeration._all_simple_cycles_iterator_edge``` + ```~sage.graphs.cycle_enumeration.all_cycles_iterator``` + ```~sage.graphs.cycle_enumeration.all_simple_cycles``` In implementing these funcionts, I fix bug in ```~sage.graphs.path_enumeration.yen_k_shortest_simple_paths```. This bug occurs when there is no path between source and target. Also, I add ```~sage.graphs.connectivity.biconnected_components_subgraphs```. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [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 and checked the documentation preview. ### ⌛ Dependencies sagemath#40248 <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40217 Reported by: Yuta Inoue Reviewer(s): David Coudert
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jun 21, 2025
sagemathgh-40248: Fix reporting weight in yen_k_shortest_simple_paths <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> If source equals target, and report_edge=True, report_weight=True, then weight is not reported in yen_k_shortest_simple_paths. This PR fixes this issue. Fix sagemath#40247. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [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 and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40248 Reported by: Yuta Inoue Reviewer(s): David Coudert
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If source equals target, and report_edge=True, report_weight=True, then weight is not reported in yen_k_shortest_simple_paths. This PR fixes this issue.
Fix #40247.
📝 Checklist
⌛ Dependencies