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

graph_plot: sort by id #22375

Closed
jdemeyer opened this issue Feb 12, 2017 · 12 comments
Closed

graph_plot: sort by id #22375

jdemeyer opened this issue Feb 12, 2017 · 12 comments

Comments

@jdemeyer
Copy link

Avoid using comparison (which may not always work, may not provide a total order, etc.) when all we want as far (as I understand) is an arbitrary total order.

CC: @mezzarobba

Component: graph theory

Author: Marc Mezzarobba

Branch/Commit: u/jdemeyer/graph_plot__sort_by_id @ 983e660

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

@jdemeyer jdemeyer added this to the sage-7.6 milestone Feb 12, 2017
@jdemeyer
Copy link
Author

Branch: u/jdemeyer/graph_plot__sort_by_id

@mezzarobba

This comment has been minimized.

@mezzarobba
Copy link
Member

Commit: 983e660

@mezzarobba
Copy link
Member

New commits:

983e660graph_plot: sort by id

@jdemeyer
Copy link
Author

comment:3

I don't get the point of this ticket. There are so many places where G.vertices() is called for a graph G. Why are all those other places ok but not this specific one that you are changing in this ticket?

@mezzarobba
Copy link
Member

comment:4

Replying to @jdemeyer:

I don't get the point of this ticket. There are so many places where G.vertices() is called for a graph G. Why are all those other places ok but not this specific one that you are changing in this ticket?

I don't really remember. But looking at the commit, it looks like this code wants to consider edges (u,v) with u ≤ v in the order in which the vertices are looped over. Sorting them by id is an easy way to avoid doing true comparisons without making deeper changes...

@jdemeyer
Copy link
Author

comment:6

Replying to @mezzarobba:

I don't really remember.

So you're asking to review a ticket when even the ticket author cannot remember the purpose of that ticket?

@mezzarobba
Copy link
Member

comment:7

Replying to @jdemeyer:

Replying to @mezzarobba:

I don't really remember.

So you're asking to review a ticket when even the ticket author cannot remember the purpose of that ticket?

I do remember that the goal was to fix problems that arose when trying to disable comparisons between objects with incompatible parents (#22029)—as you know, since it was you who wanted to make this a separate ticket. I don't remember exactly why I thought this would be a correct fix here but not elsewhere. And I'm hoping for comments from people who know this code better than I do, hence the needs_review.

@jdemeyer
Copy link
Author

comment:8

As long as you don't expect me to review it :-)

@fchapoton
Copy link
Contributor

comment:9

does not apply

@jdemeyer
Copy link
Author

jdemeyer commented Dec 7, 2017

comment:10

Any objections to closing this as "wontfix"?

@fchapoton
Copy link
Contributor

comment:11

no objection from me

@jdemeyer jdemeyer removed this from the sage-7.6 milestone Mar 29, 2018
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