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

Fix incorrect documentation from #32498 #32667

Closed
kliem opened this issue Oct 11, 2021 · 8 comments
Closed

Fix incorrect documentation from #32498 #32667

kliem opened this issue Oct 11, 2021 · 8 comments

Comments

@kliem
Copy link
Contributor

kliem commented Oct 11, 2021

The (symmetric) edge polytope of the disjoint union of graphs is the subdirect sum of the polytopes and not the product.

CC: @mantepse @orlitzky

Component: graph theory

Keywords: symmetric edge polytope

Author: Jonathan Kliem

Branch/Commit: 8eb9c50

Reviewer: Michael Orlitzky

Issue created by migration from https://trac.sagemath.org/ticket/32667

@kliem kliem added this to the sage-9.5 milestone Oct 11, 2021
@orlitzky
Copy link
Contributor

comment:2

Should these docstrings still mention connected components, or should it be something about the factors of a disjoint union?

I think the disjoint union is a special case of "connected components," and due to randomization may actually cover all such cases (up to isomorphism); but since I'm not that familiar with the result, I just want to double-check that we haven't weakened the test itself while leaving the stronger claim in the docstring.

@kliem
Copy link
Contributor Author

kliem commented Oct 13, 2021

comment:3

If you think that talking about disjoint union is more natural before the doctest, we can change it.

The previous tests were unlikely to hit graphs with non-trivial connected components. This is the reason I changed it. I tested it for various random graphs and did not detect a failure. I don't know, how I ever had the idea that it would be the product.

The result (subdirect sum) is rather easy to verify:

The edge polytope is defined as the convex hull of e_i + e_j for all edges (i,j).
The symmetric edge polytope is defined as the convex hull of e_i - e_j, e_j - e_i for all edges (i,j).

Hence each connected component has its own linear subspace to play with, which corresponds to the subdirect sum. It's not just combinatorial isomorphic, but really isomorphic up to a linear transformation (that permutes the coordinates).

@orlitzky
Copy link
Contributor

comment:4

Replying to @kliem:

If you think that talking about disjoint union is more natural before the doctest, we can change it.

The previous tests were unlikely to hit graphs with non-trivial connected components. This is the reason I changed it. I tested it for various random graphs and did not detect a failure. I don't know, how I ever had the idea that it would be the product.

No, don't change it, unless you want to fix the typo "it's" -> "its" =)

The claim is believable and that the new test should succeed is intuitively clear. So long as the change was intentional I have no problem with it.

@orlitzky
Copy link
Contributor

Reviewer: Michael Orlitzky

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2021

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

8eb9c50typo it's -> its

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2021

Changed commit from dccd8a2 to 8eb9c50

@kliem
Copy link
Contributor Author

kliem commented Oct 13, 2021

comment:6

Thanks for the review. Changed the typo.

@vbraun
Copy link
Member

vbraun commented Oct 13, 2021

Changed branch from u/gh-kliem/mistake_from_32498 to 8eb9c50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants