sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.33k stars 453 forks source link

p_group_cohomology does not build on Python3 #28414

Closed dimpase closed 4 years ago

dimpase commented 5 years ago

The aim of this ticket is to provide a new version of the optional p_group_cohomology spkg so that it builds and passes tests both with python-2 and python-3.

Hopefully the version number 3.3 (two times three) is appropriate.

Package documentation

Source ball

p_group_cohomology at Travis CI

Depends on #28444

CC: @simon-king-jena

Component: packages: optional

Author: Simon King

Branch: 6c5dafc

Reviewer: John Palmieri

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

simon-king-jena commented 5 years ago
comment:50

Replying to @dimpase:

An iterable is an object with an __iter__ method or/and __getitem__ method defined. So perhaps you can test for their presense. (These methods may be broken, but OK...)

You misunderstood. The problem was not how to test if something is iterable but how to test if that iterable is a builtin. But, as I said in my previous post, probably this isn't the actual problem anyway. What I want is to deal with strings, lists, tuples and dicts. Period. So, I could be more explicit (and I already tested that the error from comment:44 can be fixed in that way).

simon-king-jena commented 5 years ago
comment:51

In Python-2, one implemented c.__nonzero__() and then bool(c) would work. It seems that in Python-3, something else needs to be done. Is it perhaps the case that I need to implement __bool__?

What should I do to make stuff work both in py-2 and py-3? Implement both __bool__ and __nonzero__? Or would it be enough to replace __nonzero__ with __bool__?

simon-king-jena commented 5 years ago
comment:52

I found this

class test:
    def __bool__(self):
        return False
    __nonzero__=__bool__

here.

simon-king-jena commented 5 years ago
comment:53

There is a problem with the logging module: In Python-2, I get

sage: from pGroupCohomology import CohomologyRing
sage: CohomologyRing.doctest_setup()
sage: P = CohomologyRing(9,2,options='debug')
We compute this cohomology ring from scratch
Group data are rooted at '/home/king/.sage/temp/klap/19448/dir_0MUJxK/'
Computing basic setup for Small Group number 1 of order 3
Computing basic setup for Small Group number 2 of order 9
Resolution of GF(3)[9gp2]: 
          > export action matrices
H^*(SmallGroup(9,2); GF(3)): 
          Initialising maximal p-elementary abelian subgroups

whereas in Python-3 I get

sage: from pGroupCohomology import CohomologyRing
sage: CohomologyRing.doctest_setup()
sage: P = CohomologyRing(9,2,options='debug')
We compute this cohomology ring from scratch
Group data are rooted at '/home/king/.sage/temp/klap/18649/dir_9bz1_9bh/'
Computing basic setup for Small Group number 1 of order 3
Computing basic setup for Small Group number 2 of order 9
> export action matrices
Initialising maximal p-elementary abelian subgroups

So, the indentation is gone, and (whats worse) the logging doesn't indicate what object the log comes from (which I found very useful for debugging, as in the computation of a single cohomology ring often many other rings are involved).

Can someone please give me a pointer to a page explaining the differences between py-2 and py-3 in the logging module?

simon-king-jena commented 5 years ago
comment:54

Replying to @simon-king-jena:

Can someone please give me a pointer to a page explaining the differences between py-2 and py-3 in the logging module?

By looking at the code of the logging module in Python-2 and Python-3, I see where the problem comes from. In the following, self denotes an instance of pGroupCohomology.auxiliary.CohoFormatter, which sub-classes loggin.Formatter.

In Python-2, logging.Formatter.format(self, record) uses self._fmt to format the output. In Python-3, self._fmt is not used. Instead, self._style._fmt is consulted, where self._style is one of the styles in logging._STYLES.

The point is: So far, my custom self.format modifies self._fmt and then calls the default .format(). However, in Python-3, when calling the default .format(), the attribute self._fmt plays no role, but this is where I currently put the format into.

Simple way out: Do not call logging.Formatter.format in CohoFormatter.format.

But I wonder: Is there a recommended way to make the logging format change from call to call?

By this, I mean: Make the output of CohoFormatter.format(self, record) depend on record.args[0], in the sense that the output makes visible whether record.args[0] is the same object as in the previous call to CohoFormatter.format.

Or, more radically: What kind of logging approach would you recommend, what do you find most easy to comprehend/parse as a human (I am not interested in automatic parsing of the logging messages)?

I somehow like the current approach, which is: Assume that certain objects (say, ZZ and RR) are involved in a computation, and each object may issue a logging message. Then, stream of messages is arranged in blocks of messages coming from a single object, each block being prepended by an identifier of the object. I.e., if ZZ gives two messages, then RR gives three messages and then again ZZ gives a message, the output would be

Integer Ring:
    message 1
    message 2
Rational Field:
    message 3
    message 4
    message 5
Integer Ring:
    message 6
  1. Do you like this thematic arrangement of messages (which is provided by coho_logger in Python 2, but fails in Python-3)?
  2. If yes: Can you recommend a clean and easy way to achieve it both in Python 2 and Python 3? Ideally, one should still be able to include time information in the logging, which currently is not supported.
  3. If no: What logging format would you recommend?
jhpalmieri commented 5 years ago
comment:55

Instead of setting self._fmt, can you instead run self.basicConfig(format=FORMAT)? (https://docs.python.org/3/library/logging.html#logging.basicConfig) There are also ways of having several formatters (https://docs.python.org/3.7/library/logging.config.html#user-defined-objects), so maybe you can make use of that somehow.

simon-king-jena commented 5 years ago
comment:56

Replying to @jhpalmieri:

Instead of setting self._fmt, can you instead run self.basicConfig(format=FORMAT)? (https://docs.python.org/3/library/logging.html#logging.basicConfig) There are also ways of having several formatters (https://docs.python.org/3.7/library/logging.config.html#user-defined-objects), so maybe you can make use of that somehow.

From these links, I get the impression that each call to the logger results in the creation of a new formatter. Is that really the case?

jhpalmieri commented 5 years ago
comment:57

I don't know, I only looked at the links, too. It's worth trying.

(My impression from the links was that you could create several formatters and then switch from one to another on the fly.)

simon-king-jena commented 5 years ago
comment:58

Replying to @jhpalmieri:

I don't know, I only looked at the links, too. It's worth trying.

(My impression from the links was that you could create several formatters and then switch from one to another on the fly.)

Sure. But I am unsure how a change of formatter could solve the problem without impact on performance.

If logging is in "warn" mode, then there are of course still silent calls to coho_logger.debug. I want that (1) these calls are cheap (i.e., they shouldn't involve creation of Python objects) and (2) the message blocks should not take into account silent messages, i.e., a block shouldn't be closed if a silent message comes from a different object.

Of course, it should be that what I do with the formatter should rather be done with a handler, or maybe a filter. So, I will try to understand what exactly what handlers and filters do.

And, as in point 3. of comment:54: Would you recommend a different formatting of log messages?

simon-king-jena commented 5 years ago
comment:59

Currently I am reading logging HOWTO and logging Cookbook, and at the moment it seems to me that I want to use logging.LoggingAdapter, because it allows to add contextual information (which is what I'd like to use, unless people tell me a nicer way log format).

simon-king-jena commented 5 years ago
comment:60

I think I now know what to do: The customisation should be done in a Formatter, i.e., the existing solution was a good approach. Only the format() method should be different: Before calling super(CohoFormatter, self).format(record), I should add some attribute to the record that is then used in the formatter's format string.

simon-king-jena commented 5 years ago
comment:61

I think I solved the problem. I can now optionally add time information to the log, such as here:

sage: from pGroupCohomology import CohomologyRing
sage: CohomologyRing.doctest_setup()
sage: CohomologyRing.global_options('debug')
sage: CohomologyRing.global_options('time')
sage: H = CohomologyRing(720,763,prime=3)
    2019-09-18 20:54:20,277: Try to compute a Sylow 3-subgroup
    2019-09-18 20:54:20,283: Try to find the SmallGroups address of the Sylow subgroup
    2019-09-18 20:54:20,288: We compute this cohomology ring from scratch
    2019-09-18 20:54:20,289: Group data are rooted at '/home/king/.sage/temp/klap/962/dir_tnhvqeq7/'
    2019-09-18 20:54:20,293: Computing basic setup for Small Group number 1 of order 3
    2019-09-18 20:54:20,405: Computing basic setup for Small Group number 2 of order 9
Resolution of GF(3)[9gp2]:
    2019-09-18 20:54:20,466: > export action matrices
H^*(SmallGroup(9,2); GF(3)):
    2019-09-18 20:54:20,517: Initialising maximal p-elementary abelian subgroups
    2019-09-18 20:54:20,555: Computing intermediate subgroup
    2019-09-18 20:54:20,567: Try to find the SmallGroups address of the intermediate subgroup
    2019-09-18 20:54:20,603: Computing group order
    2019-09-18 20:54:20,604: The group is of order 72
    2019-09-18 20:54:20,609: Computing intermediate subgroup
Resolution of GF(3)[SmallGroup(9,2)]:
    2019-09-18 20:54:20,625: Computing next term
    2019-09-18 20:54:20,644: > rk P_02 =   3
H^*(SmallGroup(9,2); GF(3)):
    2019-09-18 20:54:20,647: We have to choose 2 new generators in degree 1
    2019-09-18 20:54:20,648: > There are 2 nilpotent generators in degree 1
...

However, what I'd find even nicer if the sum of the cputimes of Sage and Singular since start of logging was shown, not just the walltime, as above.

Note that the indentation has changed, which means that I will have to change various doc tests, but that will be doable.

simon-king-jena commented 5 years ago
comment:62

Sigh.

After solving the log problem, I find that meanwhile the code ONLY works in Python-3, not Python-2.

First problem:

class FOO(object):
    def __bool__(self):
        return something
    __nonzero__ = __bool__

works in Py3, but not in Py2, although it was recommended in some stackoverflow discussion. In Py2 it results in NameError: name '__bool__' is not defined. How to fix it without code duplication?

Second problem:

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/pGroupCohomology/factory.py in <module>()
     47 import re,os
     48 
---> 49 import urllib.request, urllib.error
     50 import tarfile
     51 import logging

ImportError: No module named request

How to deal with urllib in a py2/py3-compatible way?

jhpalmieri commented 5 years ago
comment:63

For the urllib issue: http://python3porting.com/noconv.html#import-errors suggests code like

try:
    from urllib.request import ...
except ImportError:
    from urllib import ...

I can't reproduce the first problem. With Python 2, if I do

sage: class FOO(object):
....:     def __bool__(self):
....:         return something
....:     __nonzero__ = __bool__
....:     
sage: a = FOO()
sage: a.__nonzero__()
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-3-74dab0bd3f82> in <module>()
----> 1 a.__nonzero__()

<ipython-input-1-ec93abb3d539> in __bool__(self)
      1 class FOO(object):
      2     def __bool__(self):
----> 3         return something
      4     __nonzero__ = __bool__
      5 

NameError: global name 'something' is not defined

so a.__nonzero__() is certainly calling __bool__. If I change the return line to something sensible, it works.

simon-king-jena commented 5 years ago
comment:64

Replying to @jhpalmieri:

For the urllib issue: http://python3porting.com/noconv.html#import-errors suggests code like

try:
    from urllib.request import ...
except ImportError:
    from urllib import ...

OK, thank you.

I can't reproduce the first problem. With Python 2, if I do

The "something" was of course figuratively. Anyway, the problem seems to be that I am using a cdef class here:. In Sage-with-py2:

sage: cython('''
....: class Foo:
....:     def __bool__(self):
....:         return True
....:     __nonzero__ = __bool__
....: ''')
sage: cython('''
....: cdef class Foo:
....:     def __bool__(self):
....:         return True
....:     __nonzero__ = __bool__
....: ''')
ERROR:root:An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line string', (1, 0))

---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-2-91e48d391924> in <module>()
      4         return True
      5     __nonzero__ = __bool__
----> 6 ''')

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3697)()
    352             True
    353         """
--> 354         return self.get_object()(*args, **kwds)
    355 
    356     def __repr__(self):

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython_compile(code, **kwds)
    649     with open(tmpfile,'w') as f:
    650         f.write(code)
--> 651     return cython_import_all(tmpfile, get_globals(), **kwds)

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython_import_all(filename, globals, **kwds)
    539       code
    540     """
--> 541     m = cython_import(filename, **kwds)
    542     for k, x in iteritems(m.__dict__):
    543         if k[0] != '_':

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython_import(filename, **kwds)
    519     try:
    520         sys.path.append(build_dir)
--> 521         return builtins.__import__(name)
    522     finally:
    523         sys.path = oldpath

/home/king/.sage/temp/klap/23274/spyx/_home_king__sage_temp_klap_23274_tmp_FgSymA_pyx/_home_king__sage_temp_klap_23274_tmp_FgSymA_pyx_0.pyx in init _home_king__sage_temp_klap_23274_tmp_FgSymA_pyx_0()
      3     def __bool__(self):
      4         return True
----> 5     __nonzero__ = __bool__

NameError: name '__bool__' is not defined

whereas in Sage-with-py3 I get

sage: cython('''
....: class Foo:
....:     def __bool__(self):
....:         return True
....:     __nonzero__ = __bool__
....: ''')
sage: cython('''
....: cdef class Foo:
....:     def __bool__(self):
....:         return True
....:     __nonzero__ = __bool__
....: ''')

So, what to do?

jhpalmieri commented 5 years ago
comment:65

I don't know Cython, but replacing __nonzero__ = __bool__ with

def __nonzero__(self):
    return self.__bool__()

seems to work.

simon-king-jena commented 5 years ago
comment:66

Replying to @jhpalmieri:

I don't know Cython, but replacing __nonzero__ = __bool__ with

def __nonzero__(self):
    return self.__bool__()

seems to work.

Sure. that's what I basically meant with "code duplication". It is an additional call, thus, if the boolean value of a cochain is tested very often then one will see a slow-down. But I don't know if it actually is tested very often.

Anyway, with this code cuplication, things seem to work. Hooray!

At the moment, it seems that most of the remaining work will be to change the expected output for those tests that use logging.

jhpalmieri commented 5 years ago
comment:67

My hope is that we can drop Python 2 support pretty soon, or at least make sure that Python 3 works well, even if it as the expense of performance with Python 2. Does __nonzero__ do anything with Python 3? If so, maybe you could do something like

from six import PY2
if PY2:
    def __nonzero__(self):
        ...

to avoid the extra call when using Python 3.

simon-king-jena commented 5 years ago
comment:68

Replying to @jhpalmieri:

My hope is that we can drop Python 2 support pretty soon, or at least make sure that Python 3 works well, even if it as the expense of performance with Python 2. Does __nonzero__ do anything with Python 3? If so, maybe you could do something like

from six import PY2
if PY2:
    def __nonzero__(self):
        ...

to avoid the extra call when using Python 3.

I don't think that __nonzero__ does anything in Python 3. But now as you mention it: It is possible (and is actually used in some places in the cythonised parts of the Sage library and also in the current not-yet-published code of the cohomology package) to make the code depend on the language level. I.e., I could have definitions for __nonzero__ and for __bool__, but only one of them would be compiled.

simon-king-jena commented 5 years ago
comment:69

One detail: According to unit_test_64, there is no significant speed difference between the py2 and py3 versions. In any case, the cputime obtained as sum of the cputime() and Singular's timer is only half of the walltime. Do I recall correctly that the time used to save data on disk is not measured by cputime()? Is there an easy way to determine that time, too?

dimpase commented 5 years ago
comment:70

I don't think it makes much sense measuring time to save to disk, as it varies wildly between different OS's, different hardware, different configuration of buffering, etc etc.

Often saving is completely asyncronous from the computation due to buffering and separate threads run to do buffer flushing, etc.

simon-king-jena commented 5 years ago
comment:71

Now I am totally baffled. Is it not the case that comparison is supposed to be implemented by __richcmp__? Apparently __richcmp__ isn't called anymore.

The following corresponds to the code in pGroupCohomology.dickson, that did work in the past:

sage: cython("""
....: from sage.structure.richcmp import richcmp, op_LT, op_NE, op_GT
....: from sage.all import GF
....: class DICKSON:
....:     def __init__(self,p):
....:         self.K = GF(p)
....:     def __richcmp__(self, other, op):
....:         print("Comparison called")
....:         return richcmp(self.K, other.K, op)
....: """)

Now, rich comparison fails both in py2 and py3:

sage: D = DICKSON(5)
sage: F = loads(dumps(D))
sage: D == F
False
sage: F.K == D.K
True

What has changed, how to fix it?

simon-king-jena commented 5 years ago
comment:72

Aha! I see in sage.misc.unknown that a decorator @richcmp_method is used. So, I guess I need to use it, too.

simon-king-jena commented 5 years ago
comment:73

Replying to @simon-king-jena:

Aha! I see in sage.misc.unknown that a decorator @richcmp_method is used. So, I guess I need to use it, too.

Hooray:

sage: cython("""
....: from sage.structure.richcmp import richcmp, richcmp_method
....: from sage.all import GF
....: @richcmp_method
....: class DICKSON(object):
....:     def __init__(self,p):
....:         self.K = GF(p)
....:     def __richcmp__(self, other, op):
....:         print("Comparison called")
....:         return richcmp(self.K, other.K, op)
....: """)
sage: D = DICKSON(5)
sage: F = loads(dumps(D))
sage: D == F
Comparison called
True
simon-king-jena commented 5 years ago
comment:74

At the moment, tests in dickson, auxiliaries, resolution and factory pass both in Py2 and Py3. In isomorphism_test, there are currently two failing tests, and it seems to me that they reveal a Py3-bug in cartesian_product_iterator:

File "/home/king/Projekte/coho/coho-devel/src/pGroupCohomology/isomorphism_test.py", line 353, in pGroupCohomology.isomorphism_test.IsomorphismTest.explore_isomorphisms
Failed example:
    T.explore_isomorphisms()
Exception raised:
    Traceback (most recent call last):
      File "sage/misc/cachefunc.pyx", line 1944, in sage.misc.cachefunc.CachedMethodCaller.__call__ (build/cythonized/sage/misc/cachefunc.c:10139)
        return cache[k]
    KeyError: (((3, 5, 4, 6, 7, 1, 2), 0, True), ())

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/home/king/Sage/git/py3/local/lib/python3.7/site-packages/sage/misc/mrange.py", line 139, in _xmrange_iter
        curr_elt = [next(i) for i in curr_iters[:-1]] + [None]
      File "/home/king/Sage/git/py3/local/lib/python3.7/site-packages/sage/misc/mrange.py", line 139, in <listcomp>
        curr_elt = [next(i) for i in curr_iters[:-1]] + [None]
    StopIteration

    The above exception was the direct cause of the following exception:

    Traceback (most recent call last):
      File "/home/king/Sage/git/py3/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 681, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/king/Sage/git/py3/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1123, in compile_and_execute
        exec(compiled, globs)
      File "<doctest pGroupCohomology.isomorphism_test.IsomorphismTest.explore_isomorphisms[10]>", line 1, in <module>
        T.explore_isomorphisms()
      File "/home/king/Sage/git/py3/local/lib/python3.7/site-packages/pGroupCohomology/isomorphism_test.py", line 402, in explore_isomorphisms
        Cands = self.candidates_of_gens(tuple([p[4] for p in Sizes]),len(self.rigid_generators))
      File "sage/misc/cachefunc.pyx", line 1949, in sage.misc.cachefunc.CachedMethodCaller.__call__ (build/cythonized/sage/misc/cachefunc.c:10273)
        w = self._instance_call(*args, **kwds)
      File "sage/misc/cachefunc.pyx", line 1825, in sage.misc.cachefunc.CachedMethodCaller._instance_call (build/cythonized/sage/misc/cachefunc.c:9758)
        return self.f(self._instance, *args, **kwds)
      File "/home/king/Sage/git/py3/local/lib/python3.7/site-packages/pGroupCohomology/isomorphism_test.py", line 644, in candidates_of_gens
        for Ims1,Ims2 in cartesian_product_iterator([FirstHalf, SecondHalf]):
    RuntimeError: generator raised StopIteration

I will investigate which values of FirstHalf and SecondHalf trigger it.

simon-king-jena commented 5 years ago
comment:75

Replying to @simon-king-jena:

I will investigate which values of FirstHalf and SecondHalf trigger it.

See #28521

EDIT: I am not sure if I'll make #28521 a dependency or simply work around the bug.

simon-king-jena commented 5 years ago
comment:76

Replying to @simon-king-jena:

At the moment, tests in dickson, auxiliaries, resolution and factory pass both in Py2 and Py3.

When working around the cartesian_product_iterator bug, the tests in isomorphism_test pass both in Py2 and Py3. Yay!

simon-king-jena commented 5 years ago
comment:77

Two errors in Py3 concern __div__, namely

Running doctests with ID 2019-09-21-07-26-57-20b48344.
Using --optional=build,dochtml,gap_packages,libsemigroups,meataxe,memlimit,mpir,python2,sage
Doctesting 1 file.
sage -t --long --warn-long 47.4 /home/king/Projekte/coho/coho-devel/src/pGroupCohomology/cochain.pyx
**********************************************************************
File "/home/king/Projekte/coho/coho-devel/src/pGroupCohomology/cochain.pyx", line 3131, in pGroupCohomology.cochain.MODCOCH.__div__
Failed example:
    print(H.1/3) #indirect doctest
Expected:
    (a_6_0)/(3): 6-Cocycle in H^*(SmallGroup(400,206); GF(5))
    defined by
    2*c_2_1*c_2_2*a_1_0*a_1_1
Got:
    (a_6_0)*(2): 6-Cocycle in H^*(SmallGroup(400,206); GF(5))
    defined by
    2*c_2_1*c_2_2*a_1_0*a_1_1
**********************************************************************
File "/home/king/Projekte/coho/coho-devel/src/pGroupCohomology/cochain.pyx", line 3139, in pGroupCohomology.cochain.MODCOCH.__div__
Failed example:
    H.2/H.1
Expected:
    Traceback (most recent call last):
    ...
    TypeError: Can not divide by <class 'pGroupCohomology.cochain.MODCOCH'>
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/home/king/Sage/git/py3/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 681, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/king/Sage/git/py3/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1123, in compile_and_execute
        exec(compiled, globs)
      File "<doctest pGroupCohomology.cochain.MODCOCH.__div__[6]>", line 1, in <module>
        H.gen(2)/H.gen(1)
      File "sage/structure/element.pyx", line 1718, in sage.structure.element.Element.__truediv__ (build/cythonized/sage/structure/element.c:12780)
        return (<Element>left)._div_(right)
      File "sage/structure/element.pyx", line 2654, in sage.structure.element.RingElement._div_ (build/cythonized/sage/structure/element.c:18184)
        frac = self._parent.fraction_field()
    TypeError: 'NoneType' object is not callable
**********************************************************************
1 item had failures:
   2 of   8 in pGroupCohomology.cochain.MODCOCH.__div__
    [1490 tests, 2 failures, 465.54 s]
----------------------------------------------------------------------
sage -t --long --warn-long 47.4 /home/king/Projekte/coho/coho-devel/src/pGroupCohomology/cochain.pyx  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 470.6 seconds
    cpu time: 249.3 seconds
    cumulative wall time: 465.5 seconds

The first error seems to indicate that H.1/3 is automatically translated to H.1*2, which does make sense modulo 5, but which I did not implement like this. And the second error is nothing that I implemented.

In fact I overrode __div__ for COCH and MODCOCH. Is it perhaps the case that overriding __div__ won't work in Py3?

fchapoton commented 5 years ago
comment:78

In py3, there is __truediv__ and __floordiv__

simon-king-jena commented 5 years ago
comment:79

Replying to @fchapoton:

In py3, there is __truediv__ and __floordiv__

I see. Do I understand correctly that in order to make my code Py2 and Py3 compatible, I should implement __truediv__, __floordiv__ and __div__?

On the other hand, I reckon that Sage has default implementations of the double underscore methods, that I should rather not override.

So, given a RingElement, what methods do I need to implement for providing division by an element of the base field, and what do I need to implement for making clear that division of two ring elements doesn't work?

I guess I should implement a fraction_field method for cohomology rings. Of course, since cohomology rings can have zero divisors, a fraction field will not always exist. So, should in that situation fraction_field return NotImplemented, or raise a ValueError, or what else?

Do I need to provide a single-underscore _div_ method, in addition to the fraction_field?

How to implement division by a base field element (I guess _div_ is for division of two ring elements)?

simon-king-jena commented 5 years ago
comment:80

Replying to @simon-king-jena:

Replying to @fchapoton:

In py3, there is __truediv__ and __floordiv__

I see. Do I understand correctly that in order to make my code Py2 and Py3 compatible, I should implement __truediv__, __floordiv__ and __div__?

On the other hand, I reckon that Sage has default implementations of the double underscore methods, that I should rather not override.

I think I'll proceed as follows:

simon-king-jena commented 5 years ago
comment:81

Replying to @simon-king-jena:

I don't think that __nonzero__ does anything in Python 3. But now as you mention it: It is possible (and is actually used in some places in the cythonised parts of the Sage library and also in the current not-yet-published code of the cohomology package) to make the code depend on the language level. I.e., I could have definitions for __nonzero__ and for __bool__, but only one of them would be compiled.

This did work, to some extent. But not really reliably. Therefore, to be on the safe side, I will implement both methods in both python versions, since the code duplication is only small.

simon-king-jena commented 4 years ago
comment:82

I am currently dealing with different but mathematically equivalent behaviour: Apparently, previously Singular did not always do tail reduction, i.e., some rings in the database do not have a fully reduced representation of the relation ideal. For example, here are relations for the cohomology of SmallGroup(64,14):

sage: H.rels()   # not tail reduced, result obtained with py2
['a_1_0^2',
 'a_1_0*a_1_1',
 'a_1_1^3',
 'a_2_1*a_1_0',
 'a_2_1^2+a_2_1*a_1_1^2',
 'a_1_1*a_3_3+a_2_1^2',
 'a_1_0*a_3_3+a_2_1^2',
 'a_2_1*a_3_3',
 'a_3_3^2']

versus

sage: H.rels()   # tail reduced, obtained with Py3
['a_1_0^2',
 'a_1_0*a_1_1',
 'a_1_1^3',
 'a_2_1*a_1_0',
 'a_2_1^2+a_2_1*a_1_1^2',
 'a_1_0*a_3_3+a_2_1*a_1_1^2',
 'a_1_1*a_3_3+a_2_1*a_1_1^2',
 'a_2_1*a_3_3',
 'a_3_3^2']

As you can see, the relation a_2_1<sup>2+a_2_1*a_1_1</sup>2 could be used to reduce the tail of a_1_1*a_3_3+a_2_1^2. Also, the sorting of the relations is different.

The question is how to deal with it. I guess I could proceed as follows:

sage: if (2, 8) < sys.version_info:
....:     expected_rels = ['a_1_0^2', 'a_1_0*a_1_1', 'a_1_1^3', 'a_2_1*a_1_0',
....:           'a_2_1^2+a_2_1*a_1_1^2', 'a_1_0*a_3_3+a_2_1*a_1_1^2',
....:           'a_1_1*a_3_3+a_2_1*a_1_1^2', 'a_2_1*a_3_3', 'a_3_3^2']
....: else:
....:     expected_rels = ['a_1_0^2', 'a_1_0*a_1_1', 'a_1_1^3', 'a_2_1*a_1_0',
....:           'a_2_1^2+a_2_1*a_1_1^2', 'a_1_1*a_3_3+a_2_1^2',
....:           'a_1_0*a_3_3+a_2_1^2', 'a_2_1*a_3_3', 'a_3_3^2']
....:     
sage: H.rels() == expected_rels
True
simon-king-jena commented 4 years ago
comment:83

Sigh. I am suffering from "interesting" debugging sessions. At the moment, it seems to me that some errors in the py2-version only occur, when I first install p_group_cohomology in the py2-install of Sage, then install p_group_cohomology in the py3-install of Sage, and only then do tests in the py2-install.

dimpase commented 4 years ago
comment:84

I had a bad experience with doing py2/3 things on the same account, the problems was apparently in the content of ~/.sage/.

simon-king-jena commented 4 years ago
comment:85

Replying to @dimpase:

I had a bad experience with doing py2/3 things on the same account, the problems was apparently in the content of ~/.sage/.

You mean by testing the p_group_cohomology package, or something else?

Indeed I had the same impression concerning ~/.sage/. Fortunately most of the trouble could be solved by putting str() around some data (because I got unicode where a string was expected - so, perhaps a rogue py3-pickle got misinterpreted in py2).

dimpase commented 4 years ago
comment:86

Replying to @simon-king-jena:

Replying to @dimpase:

I had a bad experience with doing py2/3 things on the same account, the problems was apparently in the content of ~/.sage/.

You mean by testing the p_group_cohomology package, or something else?

Indeed I had the same impression concerning ~/.sage/. Fortunately most of the trouble could be solved by putting str() around some data (because I got unicode where a string was expected - so, perhaps a rogue py3-pickle got misinterpreted in py2).

I was getting different output from a test, IIRC it was releated to unicode, and the problem went away after I removed ~/.sage/. I thought it might have been related to ipython, but who knowns.

simon-king-jena commented 4 years ago
comment:87

I tried to solve it by doing an explicit conversion to str whenever there is data that is expected of this type. And instead of if isinstance(bla, str), I now do if isinstance(bla, (str, unicode)), where I define unicode=str in py3.

So far, this seems to work. And after all, most users will have only one Sage installation anyway.

simon-king-jena commented 4 years ago
comment:88

Replying to @simon-king-jena:

Replying to @simon-king-jena:

If the .sobj file's location is not stored as an attribute in Python 3 then I have a problem.

Hooray:

# in Sage-with-python-2
sage: P = CombinatorialFreeModule(ZZ,'x')
sage: save(P,'../../pickle')
# BOTH with python-2 and python-3
sage: B = load('../../pickle.sobj')
sage: B._default_filename
'/home/king/Sage/pickle.sobj'

So, the old trick should still work.

I wonder if I shouldn't improve the trick.

Currently, if the initialisation of an unpickled object can not be finished, it is the __getattr__ method that would trigger the completion of initialisation based on the value of self._default_filename that is set by Sage's loads() after unpickling the object and before returning it.

But what if I turn _default_filename into a property? I could make it so that its setter triggers the completion of initialisation, to the effect that the object returned by loads() will be valid. The trickery would be in the property setter, not in __getattr__.

simon-king-jena commented 4 years ago
comment:89

I am puzzled about the appearance of unicode. For example, consider these two lines in my code:

        root = str(kwds.get('root', COHO.workspace))
        coho_logger.debug("Group data are rooted at %r", None, root)

The logging appears in the following session (in Sage-with-py2, where I first installed the package in Sage-with-py3 and then in Sage-with-py2):

sage: from pGroupCohomology import CohomologyRing
sage: CohomologyRing.doctest_setup()
sage: CohomologyRing.global_options('debug')
sage: from pGroupCohomology.cohomology import COHO
sage: _ = COHO()
<module>:
    Group data are rooted at u'/home/king/.sage/temp/klap/6618/dir_T5Im2o/'

So, apparently the variable root in the code snipped above is a unicode, although I explicitly converted it to a str. Moreover:

sage: COHO.workspace
'/home/king/.sage/temp/klap/6618/dir_T5Im2o/'
sage: type(COHO.workspace)
<type 'str'>

In other words: There is an attribute COHO.workspace of type str, and when I convert it to a str then a unicode results. WTF???

dimpase commented 4 years ago
comment:90

I don't see a full picture here, as it's unclear which sessions are in py2 and which are in py3.

It might be that iPython installs some kind of display hook that sets str to convert to unicode. There is code in src/sage/repl/rich_output that might be doing this.

That is to say that unicode you see printed might be happening due to on the fly conversion, set up by DisplayManager.

simon-king-jena commented 4 years ago
comment:91

Replying to @dimpase:

I don't see a full picture here, as it's unclear which sessions are in py2 and which are in py3.

Is there unicode in py3?

The common topic is: In some cases I get a TypeError saying that a str was expected, but got unicode. And a log shows that a variable is of type unicode after an EXPLICIT(!) cast to str.

In all mentioned cases, I proceeded as follows:

  1. Install the package in Sage-with-py2
  2. Install the package in Sage-with-py3
  3. Test in Sage-with-py2

Sage-with-py2 and Sage-with-py3 are of course in different folders. What they share: The git repository (but different git worktrees), and DOT_SAGE.

But it is all very flaky. I just inserted a couple of print statements in my code to investigate more deeply, did the above double-install procedure --- and suddenly the type was str, not unicode.

It might be that iPython installs some kind of display hook that sets str to convert to unicode. There is code in src/sage/repl/rich_output that might be doing this.

It is not only the output but the type inside of the code. In some cases, I get a TypeError.

dimpase commented 4 years ago
comment:92

Do you see these errors while running doctests? I gather there is --nodotsage option you can use while running tests, so you may try to see whether you can reproduce this without DOT_SAGE.

Another thing to try is to wipe out DOT_SAGE each time you switch from pyX to py(5-X), and see whether this make the error to go away.

dimpase commented 4 years ago
comment:93

another way to work around this might be just to convert all your strings to unicode.

simon-king-jena commented 4 years ago
comment:94

I am afraid that it is currently not possible for anybody else to reproduce the following, because I didn't publish my code yet. But anyway, here is what I did:

  1. Insert type information, so that the mentioned code snippet now is

        root = str(kwds.get('root', COHO.workspace))
        coho_logger.debug("Group data are rooted at %r. Type: %r", None, root, type(root))
  2. Install it in py3
  3. Install it in py2
  4. In Py3, run the following:

    sage: from pGroupCohomology import CohomologyRing
    sage: CohomologyRing.doctest_setup()
    sage: H = CohomologyRing(64,14, from_scratch=True)
    sage: H.make()
  5. In Py2, run the following:

    sage: from pGroupCohomology import CohomologyRing
    sage: from pGroupCohomology.cohomology import COHO
    sage: CohomologyRing.doctest_setup()
    sage: CohomologyRing.global_options('debug')
    sage: _ = COHO()
    <module>:
      Group data are rooted at u'/home/king/.sage/temp/klap/14917/dir_20mizE/'. Type: <type 'unicode'>

So, the type REALLY is unicode, even though I did an EXPLICIT conversion to str.

Then, I continued:

  1. Move the old ~/.sage and create a new one.
  2. Start Sage-with-py2, and repeat the test:

    Setting permissions of DOT_SAGE directory so only you can read and write it.
    sage: from pGroupCohomology import CohomologyRing
    sage: from pGroupCohomology.cohomology import COHO
    sage: CohomologyRing.doctest_setup()
    sage: CohomologyRing.global_options('debug')
    sage: _ = COHO()
    <module>:
      Group data are rooted at u'/home/king/.sage/temp/klap/15112/dir_fYCkwo/'. Type: <type 'unicode'>
    sage: type(COHO.workspace)
    <type 'str'>
    

So, removing DOT_SAGE didn't help. For comparison, do the same in Py3:

sage: from pGroupCohomology import CohomologyRing
sage: from pGroupCohomology.cohomology import COHO
sage: CohomologyRing.doctest_setup()
sage: CohomologyRing.global_options('debug')
sage: _ = COHO()
<module>:
    Group data are rooted at '/home/king/.sage/temp/klap/15493/dir_81mwix3u/'. Type: <class 'str'>

which of course is no surprise, as python-3 has no unicode.

Note that the line _ = COHO() does not involve taking an old pickle from a repository. It merely creates a new instance of COHO and initialises it only to the point where the current workspace is assigned to the attribute .root.

Summary: The class attribute COHO.workspace is of type str. The dictionary kwds is empty. str(kwds.get('root', COHO.workspace))) returns the unicode version of `COHO.workspace. I.e., converting a str to a str results in a unicode.

simon-king-jena commented 4 years ago
comment:95

Just to document that we are talking about a Heisenbug: I wanted to test whether it is perhaps the case that str==unicode. Therefore, I changed the code snippet to

        root = str(kwds.get('root', COHO.workspace))
        coho_logger.debug("Group data are rooted at %r. Type: %r, %r, %r", None, root, type(root), type(COHO.workspace), str)

, installed it in py2 (only), and rerun the test in py2:

sage: from pGroupCohomology import CohomologyRing
sage: from pGroupCohomology.cohomology import COHO
sage: CohomologyRing.doctest_setup()
sage: CohomologyRing.global_options('debug')
sage: _ = COHO()
<module>:
    Group data are rooted at '/home/king/.sage/temp/klap/16014/dir_M5saF0/'. Type: <type 'str'>, <type 'str'>, <type 'str'>

Hence, no bug. "Hooray".

Start a sage-with-py3-session and do

sage: from pGroupCohomology import CohomologyRing
sage: CohomologyRing.doctest_setup()
sage: H = CohomologyRing(64,14, from_scratch=True)
sage: H.make()

The hope was of course that it triggers something. But the test in py2 does not have a unicode problem. "Hooray".

But I am sure that the problem will pop up sooner or later.

simon-king-jena commented 4 years ago
comment:96

To make sure that this bug doesn't pop up again, I inserted a test whether the python version is less than (2,8) and root is of type unicode. In this case, I print some information and then raise a runtime error. I guess it's the best that I can do for now.

After recompiling and rerunning the above tests, they went fine again. But sooner or later it will return, and then the diagnostic tests I inserted will perhaps tell more clearly what is happening.

simon-king-jena commented 4 years ago
comment:97

Or another idea: Since it is Cython code, I could do cdef str root = ....

simon-king-jena commented 4 years ago
comment:98

Hooray!

The inserted test already showed something:

sage: from pGroupCohomology import CohomologyRing
sage: from pGroupCohomology.cohomology import COHO
sage: CohomologyRing.doctest_setup()
sage: CohomologyRing.global_options('debug')
sage: _ = COHO()
Critical bug in python.
kwds.get('root', COHO.workspace) is <type 'str'>
COHO.workspace is <type 'str'>
str=<type 'unicode'>, unicode=<type 'unicode'>
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-5-a303ce041f27> in <module>()
----> 1 _ = COHO()

pGroupCohomology/cohomology.pyx in pGroupCohomology.cohomology.COHO.__init__()

RuntimeError: An explicit cast to <str> resulted in a unicode object

Aha! For some silly reason, we have str == unicode!!

EDIT: The above was printed like this:

        if (2, 8) > sys.version_info and isinstance(root, unicode):
            print("Critical bug in python.")
            print("kwds.get('root', COHO.workspace) is %r"%type(kwds.get('root', COHO.workspace)))
            print("COHO.workspace is %r"%type(COHO.workspace))
            print("str=%r, unicode=%r"%(str,unicode))
            raise RuntimeError("An explicit cast to <str> resulted in a unicode object")
simon-king-jena commented 4 years ago
comment:99

How to cope with it? Shall I test upon importing of the module whether str == unicode in Python-2, and raise an error whose message tells to recompile?