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
Graphs: docstring of _add_ conflicts with function #20499
Comments
comment:1
David, a question. How should this function work?
|
comment:2
The current implementation of the
So it forces to relabel vertices as integer in At the least, we should raise an error for cases such as
Now, the semantic of Concerning the relabel to integers part, I agree that this is often painful. We could propose and intermediate behavior like: relabel only if some vertices have same label. But again some people will complain. David. |
comment:3
It seems that example given uses consecutive integeres. But yes, your suggestions sounds reasonable. But now I see this:
Maybe the |
comment:4
Right, we could improve the naming part of
I just saw that there is a deprecation warning in |
comment:5
Should I open a discussion about this in sage-devel? |
comment:6
That's a good idea. |
comment:7
Replying to @dcoudert:
I like the idea of only relabeling (the entire graph) if there are common vertices. NetworkX automatically relabels everything to integers when adding graphs and they get along fine without people complaining. We could possibly think about doing something with recording the previously named values when relabeling them. That way people could choose to invert the labeling if it was really necessary. Our add function could pass in a dictionary of the previous labels as an attribute in the newly created graph. |
comment:9
Replying to @sagetrac-mcognetta:
OK for me. Then it would be like And then there is the naming thing. But first I will wait for three days to see if there will be opinions against this. |
Commit: |
Author: Jori Mäntysalo |
comment:13
We could using something like that, which is valid for both
|
Reviewer: David Coudert |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:15
Replying to @dcoudert: I didi three changes as you suggested.
This will give an error: immutable graph can not change name. I suggest we keep this as it is now and think about this later. |
comment:16
The error message is confusing since the order of types is the reverse of the one used in the operation.
I was not aware of the problem with immutable graphs. So ok to assign directly to |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:18
OK, done that. (But I think that naming question should be considered later. Maybe the design can be made better.) |
comment:19
The patch is good to go. |
Changed branch from u/jmantysalo/graphs__docstring_of__add__conflicts_with_function to |
(Graph({'a': []})+Graph({'b': []})).vertices()
gives[0, 1]
, but docstring of__add__
says "If there are common vertices to both, they will be renamed." I do not know which way it should be.Also there is no error checking, so
Graph({0:[]})+'junk'
does and returns nothing.CC: @dcoudert @sagetrac-mcognetta
Component: graph theory
Author: Jori Mäntysalo
Branch/Commit:
2efa40a
Reviewer: David Coudert
Issue created by migration from https://trac.sagemath.org/ticket/20499
The text was updated successfully, but these errors were encountered: