kenzo package and interface

miguelmarco commented 5 years ago

Add a kenzo optional package, and the corresponding interface.

The source file is taken from https://github.com/miguelmarco/kenzo, which is a small fork by Miguel Marco to include some utility scripts from https://github.com/gheber/kenzo, which is in turn a fork of the original source code modified to be compatible with other lisps (in particular it works on ecl).

Tarball (to be renamed kenzo-1.1.7-r1.tar.gz):

https://github.com/miguelmarco/kenzo/archive/1.1.7-r1.tar.gz

(Miguel Marco is maintaining a fork for the moment, this version includes a script to compile kenzo into a .fas file).

The interface uses the ecl interface, and provides a wrapper for some kenzo objects (we hope to extend it in the future).

CC: @jhpalmieri @slel @dimpase

Component: interfaces

Keywords: upgrade, kenzo, topology

Author: Miguel Marco, Ana Romero

Branch: 4a164a8

Reviewer: Travis Scrimshaw

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

miguelmarco commented 5 years ago

Changed keywords from none to kenzo topology

miguelmarco commented 5 years ago

Description changed:

--- 
+++ 
@@ -1 +1,9 @@
+Add a kenzo optional package, and the corresponding interface.

+The source file is taken from https://github.com/gheber/kenzo , which is a fork of the original source code modified to be compatible with other lisps (in particular it works on ecl).
+
+The file used is https://github.com/gheber/kenzo/archive/master.zip (I have asked the author to make a recent release).
+
+The interface uses the ecl interface, and provides a wrapper for some kenzo objects (we hope to extend it in the future).
+
+

miguelmarco commented 5 years ago

Author: Miguel Marco, Ana Romero

miguelmarco commented 5 years ago

Branch: u/mmarco/kenzo_package_and_interface

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

b54179a Add optional tag to doctests

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Commit: b54179a

jhpalmieri commented 5 years ago

comment:5

This is exciting to see. Can we get an interface between Kenzo and Sage's implementation of simplicial sets? A crude version is in sage.homology.simplicial_set_examples.simplicial_data_from_kenzo_output, which was written to handle the files in src/ext/kenzo. But doing it in lisp or Python rather than just parsing strings would be much better.

miguelmarco commented 5 years ago

comment:6

That is the long term idea we had in mind for starting this; but we still haven't decided the design.

Maybe include an attribute for each Sage simplicial set that is a reference to the corresponding kenzo object (via this interface)?

Right now we have just coded the bare minimum kenzo functionality, but for that task maybe we should add support for explicit list of faces and maps.

miguelmarco commented 5 years ago

comment:7

I just noticed that the code I pushed in this branch fails when trying to load it. It works ok in my install of Sage based on python3, but fails in the standard python2. Since we don't know how much time will take until python3 makes it into standard sage installs, I will try to find a way to import kenzo inside sage's ecl that works also for python2.

In the meantime, you can test this branch using a python3 based sage, following the instructions in https://groups.google.com/forum/#!topic/sage-devel/uEHBKekJ_Cw

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

76e6ed3 Load asdf before requiring it to prevent problem with python2

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from b54179a to 76e6ed3

miguelmarco commented 5 years ago

comment:9

I just pushed a one liner that prevents the error with python2 (at least in my system). Please test it.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

ad33c8a Make fix specific for python2

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 76e6ed3 to ad33c8a

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

a3f051b Compile into .fas file

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from ad33c8a to a3f051b

miguelmarco commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,8 +1,8 @@
 Add a kenzo optional package, and the corresponding interface.

-The source file is taken from https://github.com/gheber/kenzo , which is a fork of the original source code modified to be compatible with other lisps (in particular it works on ecl).
+The source file is taken from https://github.com/mmarco/kenzo , which is a small fork i made to include some utility scripts from https://github.com/gheber/kenzo, which is in turn a fork of the original source code modified to be compatible with other lisps (in particular it works on ecl).

-The file used is https://github.com/gheber/kenzo/archive/master.zip (I have asked the author to make a recent release).
+The file used is https://github.com/miguelmarco/kenzo/archive/1.1.7-r1.tar.gz (I have decided to mantain a fork for the moment, this version includes a script to compile kenzo into a .fas file).

 The interface uses the ecl interface, and provides a wrapper for some kenzo objects (we hope to extend it in the future).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from a3f051b to aae2671

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

aae2671 Switch to forked upstream with utility scripts

miguelmarco commented 5 years ago

comment:14

Switched to another upstream source. It is a fork I made to be able to include some utility scripts that we might need.

For the moment, it includes a script to compile all kenzo in a self-contained .fasl file, that we can easily load without problems both from python2 and python3.

I think it is pretty much ready to be tested.

miguelmarco commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,6 @@
 Add a kenzo optional package, and the corresponding interface.

-The source file is taken from https://github.com/mmarco/kenzo , which is a small fork i made to include some utility scripts from https://github.com/gheber/kenzo, which is in turn a fork of the original source code modified to be compatible with other lisps (in particular it works on ecl).
+The source file is taken from https://github.com/miguelmarco/kenzo , which is a small fork i made to include some utility scripts from https://github.com/gheber/kenzo, which is in turn a fork of the original source code modified to be compatible with other lisps (in particular it works on ecl).

 The file used is https://github.com/miguelmarco/kenzo/archive/1.1.7-r1.tar.gz (I have decided to mantain a fork for the moment, this version includes a script to compile kenzo into a .fas file).

slel commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,6 @@
 Add a kenzo optional package, and the corresponding interface.

-The source file is taken from https://github.com/miguelmarco/kenzo , which is a small fork i made to include some utility scripts from https://github.com/gheber/kenzo, which is in turn a fork of the original source code modified to be compatible with other lisps (in particular it works on ecl).
+The source file is taken from https://github.com/miguelmarco/kenzo, which is a small fork i made to include some utility scripts from https://github.com/gheber/kenzo, which is in turn a fork of the original source code modified to be compatible with other lisps (in particular it works on ecl).

 The file used is https://github.com/miguelmarco/kenzo/archive/1.1.7-r1.tar.gz (I have decided to mantain a fork for the moment, this version includes a script to compile kenzo into a .fas file).

dimpase commented 5 years ago

comment:17

we could add your scripts to @gheber's repo, provided they are sufficiently generic; after all I already did a bit of work on it: https://github.com/gheber/kenzo/graphs/contributors :-)

miguelmarco commented 5 years ago

comment:18

Replying to @dimpase:

we could add your scripts to @gheber's repo, provided they are sufficiently generic; after all I already did a bit of work on it: https://github.com/gheber/kenzo/graphs/contributors :-)

We discussed it with gheber and Sergerarert in https://github.com/gheber/kenzo/issues/129 . We finally agreed to fork it to work in whatever we need to add (we might need to add some extra functions to allow the creation of Kenzo objects from the corresponding Sage SimplicialSets counterparts). Then maybe, when we have something stable, we can consider merging back.

miguelmarco commented 5 years ago

comment:19

I think this can be reviewed now. We still have to adapt the SimplicialSets module, and maybe write some kenzo code, but that would be better done in future tickets.

tscrim commented 5 years ago

comment:22

The documentation in your interface file needs a little more work. Some functions do not have doctests and some others have malformatted docstrings (e.g., missing EXAMPLES:: and indentations).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

3129227 Improved documentation and minor fixes

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from aae2671 to 3129227

miguelmarco commented 5 years ago

comment:24

I added more documentation. Didn't add doctest for __init__ method though. Do you think that would be necessary?

tscrim commented 5 years ago

comment:25

Thank you. This is improving, but there are a few more specific things to be fixed (although with these done, it should be at a positive review).

Yes, __init__ needs to have a doctest. I think it is the best place to put the TestSuite(foo).run() test. I also would move the INPUT: block to the class level docstring as the __init__ docstring is not in the documentation.

Change __repr__ -> _repr_ (which allows you to run foo.rename('bar') on it).

There are still some malformatted docstrings:

 def Sphere(n):
     r"""
-    Construct the n dimensional sphere, which is a simplicial set.
+    Return the ``n`` dimensional sphere as a Kenzo simplicial set.

     INPUT:

-    - ``n`` - The dimension of the sphere
+    - ``n`` -- the dimension of the sphere

-def MooreSpace(n, m):
+def MooreSpace(m, n):
     r"""
-    Construct the Moore space ``M(m, n)``. That is, the space
-    that has n'th homology group isomorphic to the cyclic group of order ``m``,
-    and the rest of the homology groups are trivial. It is a simplicial set.
+    Return the Moore space `M(m, n)` as a Kenzo simplicial set.

+    The Moore space `M(m, n)` is the space whose `n`-th homology group
+    is isomorphic to the cyclic group of order `m`, and the rest of
+    the homology groups are trivial.

     INPUT:

-    - ``m```- A positive integer. The order of the nontrivial homology group.
+    - ``m`` -- a positive integer; the order of the nontrivial homology group

-    - ``n```- The dimension in which the homology is not trivial
+    - ``n`` -- the dimension in which the homology is not trivial

Similar comments for EilenbergMacLaneSpace. (I opted for return for the short descriptions, but construct is okay with me as well.)

-        - ``kenzoobject``- a wrapper around a kenzo chain complex
-        (which is an ecl object).
+        - ``kenzoobject`` -- a wrapper around a kenzo chain complex
+          (which is an ecl object)

Similar comment for loop_space(), and a stray ` there as well: - `A KenzoSimplicialGroup.

For the doctests (e.g., in loop_space()), it is not so important to test the type(l2) but it would be more useful to see either the output or a meaningful test. For example, in the suspension doctest, the result would have a homology of Z only in the 4th degree (since the homology group (= homology group in this case) for e2 (why not call it e3?) is Z in the 3rd degree).

miguelmarco commented 5 years ago

comment:26

The testsuite fails at pickling, because we are wrapping ecl objects, that can't be pickled (maybe that is a good reason for not inheriting from SageObject? but from EclObject? or nothing at all?)

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

46150ee Improved doctests

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 3129227 to 46150ee

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

f50e18f Missing space

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 46150ee to f50e18f

jhpalmieri commented 5 years ago

comment:29

Replying to @miguelmarco:

The testsuite fails at pickling, because we are wrapping ecl objects, that can't be pickled (maybe that is a good reason for not inheriting from SageObject? but from EclObject? or nothing at all?)

You could instead skip that part of the test suite (TestSuite(foo).run(skip='_test_pickling').

There are benefits to inheriting from SageObject, so I would rather skip part of the test suite than not inherit from this.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

0fb0559 Add testsuite without pickling test

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from f50e18f to 0fb0559

miguelmarco commented 5 years ago

comment:31

Replying to @jhpalmieri:

You could instead skip that part of the test suite (TestSuite(foo).run(skip='_test_pickling').

There are benefits to inheriting from SageObject, so I would rather skip part of the test suite than not inherit from this.

Thanks for the hint!

tscrim commented 5 years ago

comment:32

A few more comments:

-# Redirection of ECL and Maxima stdout to /dev/null
+# Redirection of ECL and Kenzo stdout to /dev/null
+# This is also done in the Maxima library, but we
+#   also do it here for redundancy.

I believe Ana should also be on the copyright. Also, the modern copyright statement is

# ****************************************************************************
#       Copyright (C) 2019 Miguel Marco <mmarco@unizar.es>
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 2 of the License, or
# (at your option) any later version.
#                  https://www.gnu.org/licenses/
# ****************************************************************************

which should not be in the docstring.

MooreSpace, EilenbergMacLaneSpace and KenzoChainComplex have malformatted docstrings., e.g.:

-        - ``kenzoobject``- a wrapper around a kenzo chain complex
-        (which is an ecl object).
+        - ``kenzoobject`` -- a wrapper around a kenzo chain complex
+          (which is an ecl object)

-    - ``G`` -- A group. Currently only ``ZZ`` and the group of two elements are
-    supported.
+    - ``G`` -- group; currently only ``ZZ`` and the group of two elements are
+      supported

-    - ``n`` -- The dimension in which the homotopy is not trivial
+    - ``n`` -- the dimension in which the homotopy is not trivial

Also, in this particular docstring, there is not a unique group of 2 elements. Moreover, does it have to be GF(2)? Does G need to be additive or can it be multiplicative?

In the tests for EilenbergMacLaneSpace, it is better to use G is ZZ.

-Wraper to kenzo chain complexes
+Wrapper to kenzo chain complexes.

For future-proofing, it probably would be better to have a KenzoObject class above KenzoChainComplex with the __init__ and _repr_ methods.

It is more pythonic to do repr(self._kenzo) instead of self._kenzo.__repr__(). Similarly, kenzo_object instead of kenzoobject for variable names.

KenzoSimplicialGroup -> :class:`~sage.interfaces.kenzo.KenzoSimplicialGroup` (it might be possible to just do :class:`KenzoSimplicialGroup`) and similar changes. This is less important though.

miguelmarco commented 5 years ago

comment:33

Replying to @tscrim:

Also, in this particular docstring, there is not a unique group of 2 elements. Moreover, does it have to be GF(2)? Does G need to be additive or can it be multiplicative?

We could allow whatever we want, but since usually the homology groups are denoted additively, I used that convention. It is true however that we have so many possible incarnations of finite cyclic groups (CyclicPermutationGroup, AbelianGroup, AdditiveAbelianGroup, FinitelyPresentedGroup...) that at some point we should try to unify somehow all that, but that is definietly work for another ticket.

What would you suggest for this case? Just accept every group of cardinality two?

KenzoSimplicialGroup -> :class:`~sage.interfaces.kenzo.KenzoSimplicialGroup` (it might be possible to just do :class:`KenzoSimplicialGroup`) and similar changes. This is less important though.

I am not sure I know how to do that.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

0d2473e Split general KenzoObject class

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 0fb0559 to 0d2473e

tscrim commented 5 years ago

comment:35

Replying to @miguelmarco:

Replying to @tscrim:

Also, in this particular docstring, there is not a unique group of 2 elements. Moreover, does it have to be GF(2)? Does G need to be additive or can it be multiplicative?

We could allow whatever we want, but since usually the homology groups are denoted additively, I used that convention. It is true however that we have so many possible incarnations of finite cyclic groups (CyclicPermutationGroup, AbelianGroup, AdditiveAbelianGroup, FinitelyPresentedGroup...) that at some point we should try to unify somehow all that, but that is definietly work for another ticket.

In some sense we have that (see below). Although in general that is a can of worms (mainly, how to differentiate between additive and multiplicative groups).

What would you suggest for this case? Just accept every group of cardinality two?

We can check things abstractly since it gets fed off to a Kenzo object. Specifically, I am thinking of this:

elif G in CommutativeAdditiveGroups() and G.cardinality() == 2:

Also you should add a note to the doc saying that the group must be additive.

KenzoSimplicialGroup -> :class:`~sage.interfaces.kenzo.KenzoSimplicialGroup` (it might be possible to just do :class:`KenzoSimplicialGroup`) and similar changes. This is less important though.

I am not sure I know how to do that.

It is just some replacements in the doc to add links to the appropriate classes so it looks a little better. ;)

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

12b6026 doc fixes

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 0d2473e to 12b6026

miguelmarco commented 5 years ago

comment:37

I think I took care of all.

tscrim commented 5 years ago

comment:38

Thank you, but there are a few more little bits.

See comment:32 on MooreSpace INPUT: block.

The INPUT: for KenzoObject is over indented.

It would be good to put the one-line description for EilenbergMacLaneSpace on its own line (I also thought the convention for Eilenberg-MacLane spaces was to be with a capital K?). I would prefer the more "standard" (in the sense of the dev guide) docstring (see comment:32), but at least it is not malformatted. So this will not hold up a positive review.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 12b6026 to b95f759

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

b95f759 Minor doctest fixes

sagemath / sage

kenzo package and interface #27188