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

py3 error in graphs #23823

Closed
fchapoton opened this issue Sep 10, 2017 · 31 comments
Closed

py3 error in graphs #23823

fchapoton opened this issue Sep 10, 2017 · 31 comments

Comments

@fchapoton
Copy link
Contributor

Using a python3 build, one gets

----> 1 from sage.graphs.graph import Graph

/home/chapoton/sage3/local/lib/python3.6/site-packages/sage/graphs/graph.py in <module>()
   7870     Graph.tutte_polynomial          : "Algorithmically hard stuff",
   7871     Graph.lovasz_theta              : "Leftovers",
-> 7872     Graph.strong_orientations_iterator : "Orientations"
   7873     }
   7874 

TypeError: unhashable type: 'instancemethod'

CC: @embray @jdemeyer @tscrim @kiwifb

Component: python3

Author: Frédéric Chapoton

Branch: 5aa384a

Reviewer: David Coudert, Travis Scrimshaw

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

@fchapoton
Copy link
Contributor Author

comment:2

This comes from the lines

src/sage/graphs/graph.py:_additional_categories = {

src/sage/graphs/graph.py:__doc__ = __doc__.replace("{INDEX_OF_METHODS}",gen_thematic_rest_table_index(Graph,_additional_categories))

that try to make a dictionary where keys are instancemethods.

@fchapoton
Copy link
Contributor Author

comment:3

Trying to fix that, one meets a strange phenomenon in

sage: from sage.misc.rest_index_of_methods import *
sage: list_of_subfunctions(Graph)[0]

namely, there is just one line that is

<built-in function is_asteroidal_triple_free>

everybody else is an unbound method.

@fchapoton
Copy link
Contributor Author

Commit: 1387089

@fchapoton
Copy link
Contributor Author

New commits:

1387089tentative

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/23823

@tscrim
Copy link
Collaborator

tscrim commented Sep 20, 2017

comment:5

I think it understand what makes that special: it is a "method" defined in a pyx file whereas the others are all py files. So it is behaving a little differently.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 21, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

2fe1a1atrac 23823 replace by name
7dbe489trac 23823 almost working

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 21, 2017

Changed commit from 1387089 to 7dbe489

@fchapoton
Copy link
Contributor Author

comment:7

My branch is almost working, except for issues with cliques_maximum (duplicated) and "is_asteroidal_..." missing.


New commits:

2fe1a1atrac 23823 replace by name
7dbe489trac 23823 almost working

@fchapoton
Copy link
Contributor Author

comment:8

I am also very unsure about using __func__ in python3..

@dcoudert
Copy link
Contributor

comment:10

Replying to @tscrim:

I think it understand what makes that special: it is a "method" defined in a pyx file whereas the others are all py files. So it is behaving a little differently.

Several of these methods are defined in .pyx files: pathwidth in vertex_separation.pyx, chromatic_polynomial in chrompoly.pyx, etc. So there might be something else to explain the difference.

@embray
Copy link
Contributor

embray commented Oct 6, 2017

comment:11

I think you should be able to just write f.__name__ instead of f.__func__.__name__. Bound method objects still have a .__name__ attribute that should be the same as the underlying function's name, and the way this code is written it shouldn't be seeing classmethod or staticmethod objects (since it uses getattr on the parent object).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2017

Changed commit from 7dbe489 to 368cd00

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

f3ca592Merge branch 'u/chapoton/23823' in 8.1.b7
368cd00trac 23823 not using __func__

@fchapoton
Copy link
Contributor Author

comment:13

This is now my blocking point in the experimental python3 build.

@dcoudert
Copy link
Contributor

Reviewer: David Coudert

@dcoudert
Copy link
Contributor

comment:14

On my side, the patch passes tests, the html doc builds properly and it displays nicely in my browser. But I'm not using py3.

What else should we do to finalize the review ? Actually, this patch is still in new status, so I don't know if it should or not be reviewed...

@fchapoton
Copy link
Contributor Author

comment:15

Well, there are two problems:

  • this does not fix the issue in python3

  • I am not sure that the doc is exactly the same as before (no missing function ?)

@dcoudert
Copy link
Contributor

comment:16
  • I am not sure that the doc is exactly the same as before (no missing function ?)
  • strong_orientations_iterator moved from Orientations to Connectivity, orientations, trees
  • cliques_maximum() moved from Clique-related methods to Unsorted

Other methods are at the same place.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

5114f67Merge branch 'u/chapoton/23823' in 8.1.rc0
5aa384atrac 23823 doc-sort-key for cliques_maximum

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2017

Changed commit from 368cd00 to 5aa384a

@fchapoton
Copy link
Contributor Author

comment:18

Maybe we could validate this as a first approximation, and keep python3-full-support for another ticket ?

@tscrim
Copy link
Collaborator

tscrim commented Nov 13, 2017

comment:20

[Edit: sorry wrong ticket!]

@tscrim
Copy link
Collaborator

tscrim commented Nov 13, 2017

Changed reviewer from David Coudert to David Coudert, Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Nov 13, 2017

comment:21

IMO, I don't like Python3's behavior with this, but nothing we can do about that. I think this is an acceptable way forward, and we can always revisit this later.

@tscrim
Copy link
Collaborator

tscrim commented Nov 13, 2017

Author: Frédéric Chapoton

@embray
Copy link
Contributor

embray commented Nov 13, 2017

comment:22

This is a bit strange--I don't think this is really a "Python 3" behavior per se. But I think the change makes sense if nothing else because it avoids the issue of whether or not the attributes that are keyed on are hashable types which, depending on implementation, they aren't necessarily.

@vbraun
Copy link
Member

vbraun commented Dec 11, 2017

Changed branch from u/chapoton/23823 to 5aa384a

@fchapoton
Copy link
Contributor Author

comment:24

This needs a follow-up ticket, as the line

src/sage/graphs/graph.py:
__doc__ = __doc__.replace({INDEX_OF_METHODS}",gen_thematic_rest_table_index(Graph,
_additional_categories))

is one of the last 2 points preventing sage to start with python3 on my machine.

EDIT: @embray, did you fix this in your branch, and if yes, how ??

@fchapoton
Copy link
Contributor Author

Changed commit from 5aa384a to none

@fchapoton
Copy link
Contributor Author

comment:25

follow up in #24728

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

5 participants