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

added note for the triangle inequality case in TSP #6995

Merged

Conversation

Schefflera-Arboricola
Copy link
Contributor

@Schefflera-Arboricola Schefflera-Arboricola commented Oct 7, 2023

Closes #6748

Added the following note in the docstring of traveling_salesman_problem() function:

(Note: If the graph contains any triangle inequalities then it might return a minimal 
approximate cycle with repeating nodes(other than the starting node).)

should we also put a warning message here? because TSP makes sense only for valid graphs, without triangle inequalities. Please let me know what you think.

Thank you :)

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

I took the liberty of refactoring the suggestion so that it satisfies both the docstring standard and the formatting rules.

Comment on lines 276 to 278
If the input graph `G` contains edges whose weights do not satisfy the
triangle inequality, then the returned path may contain repeating nodes
(other than the starting node).
Copy link
Member

Choose a reason for hiding this comment

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

We should also include the case where the graph is not a complete graph (where the triangle inequality definitely doesn't apply -- non-existing edges are length inf so definitely violate the inequality). Perhaps add a phrase like: e.g. when a graph is not a complete graph.

And perhaps this should appear at the end of the paragraph at line 222 instead of in a separate Notes section.

@Schefflera-Arboricola
Copy link
Contributor Author

@dschult Thank you for the review. I've made the changes. Please let me know if this looks good.

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks!

@dschult
Copy link
Member

dschult commented Nov 9, 2023

To get this to pass the documentation build tests, we need to include a change to the testing system that has laready been merged into upstream main branch. If you want an exercise in git, you can try to "rebase" this PR onto the current main branch. That means you get git to apply the commits from this PR to the current main branch and make this branch point to the new "rebased" code.

Sometimes rebasing is as easy as: git pull upstream main --rebase.
But if the upstream main changes lines that you have also changed, you'll have to update the commits in this PR to work with applying them to the new main branch. That is called "resolving the conflicts". Sometimes that is quite time consuming. I think this one will be "easy".

But I'm confident that we can just merge this and the documentation will work. This PR only changed some words in the docs -- not it's structure or format. :)

@rossbar rossbar force-pushed the docs-TSP-triangle-inequality-case branch from d91090b to 949b041 Compare November 9, 2023 07:05
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

Excellent, thanks @Schefflera-Arboricola for the updates and @dschult for the review & suggestions!

@rossbar rossbar merged commit e962608 into networkx:main Nov 9, 2023
36 of 38 checks passed
@jarrodmillman jarrodmillman added this to the 3.3 milestone Nov 9, 2023
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Travelling Salesman Problem gives wrong results if triangle inequality is not fulfilled
4 participants