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

Cannot use integers as vertex names #693

Closed
theCapypara opened this issue Jul 7, 2023 · 7 comments
Closed

Cannot use integers as vertex names #693

theCapypara opened this issue Jul 7, 2023 · 7 comments
Milestone

Comments

@theCapypara
Copy link
Contributor

Describe the bug
Version 0.10.5 of python-igraph introduced a breaking change: It is no longer possible to use integers as vertex names.
In previous versions this was allowed and functioned, I see nothing in the documentation for add_vertex that indicates that this wasn't allowed. This change breaks existing code that used integers as vertex names.

To reproduce
Use python-igraph 0.10.5, create a simple graph and try to add a vertex with a name that is an integer. The following exception is raised:

File ".../site-packages/igraph/basic.py", line 61, in _add_vertex
    raise TypeError("cannot use integers as vertex names; use strings instead")
TypeError: cannot use integers as vertex names; use strings instead

Version information
python-igraph 0.10.5

@theCapypara
Copy link
Contributor Author

Related issue: #664

theCapypara added a commit to SkyTemple/ExplorerScript that referenced this issue Jul 7, 2023
This is breaking decompilation. Fixed by pinning version for now.

Upstream issue: igraph/python-igraph#693

SkyTemple issue: SkyTemple/skytemple-files#378
@theCapypara
Copy link
Contributor Author

I would suggest reverting this change, changing it to a deprecation warning instead and then raising the exception at earliest in version 0.11.x.

@szhorvat
Copy link
Member

szhorvat commented Jul 13, 2023

The problem is that there is no disambiguation mechanism to distinguish between vertex IDs and vertex names. When vertex names are integers, they cannot be referred to in certain contexts. Integers will just be interpreted as IDs. People often do not realize this and are getting invalid results.

@theCapypara Are you sure that this sort of ID-vs-name confusion never happens in your code?

The most desirable solution would be implementing a disambiguation mechanism, but this is a complicated long term project that would need to be a more severely breaking change.

There is no doubt that lookup-by-integer-name is desirable, but it seems to me that the current limitations make this infeasible.

If you simply need to associate an integer with each vertex, but you do not need to look up vertices by that integer, then use a different attribute than name.

P.S. Please don't take this comment as a statement on how this issue will be resolved.

@theCapypara
Copy link
Contributor Author

Thanks for the response!

I'm pretty sure I had no issues with name/ID confusion in the past. The code in question* only adds vertices and traverses the graph via the edges IIRC, so maybe that's why I haven't really noticed any issues here. Internally everything seems to still work correctly and from what I remember the vertex IDs and names were seperate and not the same.
But I'm not 100% sure because to be honest the code is pretty complex at this point and I haven't touched it in 3 years, which is why I'm hesitant to just blindly convert all the vertex names to strings and hope everything still works.

To be clear, I don't really need vertex names as integers, I can spend some time fixing this code up to use string names, it's just that this update now resulted in the code immediately breaking. I don't think igraph uses semantic versioning, but this change isn't even documented in the changelog, and it would be nice if patch-level updates still don't break existing code, semantic versioning or not.

*: it's part of a decompiler for an obscure machine language.

@szhorvat
Copy link
Member

@theCapypara Taking a practical approach here, after a cursory look at your code, it seems to me that you are not making use of the special features of the name vertex attribute. If so, can you simply use a different attribute? Instead of add_vertex(name=123) you can use add_vertex(myid=123) (or whatever attribute name you like). This would fix the issue with a very small change. You do not need the complication of using string names.


We will discuss the consequences of the breaking change, and what the best approach is, internally.

@theCapypara
Copy link
Contributor Author

Thanks! I'll definetly do that.

ntamas added a commit that referenced this issue Jul 13, 2023
@ntamas
Copy link
Member

ntamas commented Jul 13, 2023

A decision has been made to revert the change and turn it into a deprecation warning for 0.10.6; the behaviour will change in 0.11.0. A new release will come soon.

@ntamas ntamas added this to the 0.10.6 milestone Jul 13, 2023
@ntamas ntamas closed this as completed Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants