Replace most of sage.libs.gap with gappy #31404
Comments
|
Last 10 new commits:
|
|
Branch: u/embray/gappy-without-wrappers |
|
Commit: |
|
comment:2
If eventually approved, this would supersede #31297. |
This comment has been minimized.
This comment has been minimized.
|
comment:3
Does sphinx link such as
For diff --git a/build/pkgs/pplpy/spkg-install b/build/pkgs/pplpy/spkg-install
index deba1bb42b..5a39f43925 100644
--- a/build/pkgs/pplpy/spkg-install
+++ b/build/pkgs/pplpy/spkg-install
@@ -1 +1,14 @@
cd src && sdh_pip_install .
+
+if [[ "$SAGE_SPKG_INSTALL_DOCS" != no ]] ; then
+ # Compile pplpy's documentation
+ sage-python23 setup.py build_ext --inplace
+ PYTHONPATH=$PYTHONPATH:$(pwd)
+ cd docs
+ $MAKE html
+
+ # install pplpy's documentation
+ PPLPY_DOCS=$SAGE_SHARE/doc/pplpy
+ mkdir -p $PPLPY_DOCS
+ cp -r build/html/* $PPLPY_DOCS
+fi
diff --git a/src/doc/common/conf.py b/src/doc/common/conf.py
index 91f651a41c..70dd17f271 100644
--- a/src/doc/common/conf.py
+++ b/src/doc/common/conf.py
@@ -177,7 +177,8 @@ python_version = sys.version_info.major
intersphinx_mapping = {
'python': ('https://docs.python.org/',
os.path.join(SAGE_DOC_SRC, "common",
- "python{}.inv".format(python_version)))}
+ "python{}.inv".format(python_version))),
+ 'pplpy': (os.path.join(SAGE_SHARE, "doc", "pplpy"), None)}
def set_intersphinx_mappings(app):
"""What we did is not robust at all... |
|
comment:4
I think the Python package |
|
comment:5
I am still working on updating the docs. |
|
comment:6
Replying to @mkoeppe:
I opened a thread on sage-devel, no need to clutter the ticket with general documentation issue. |
|
comment:7
Another tricky aspect of the doc build: When I try to add a spkg-postinst to install the docs for gappy, gappy's own conf.py has an intersphinx mapping for the Python docs. Even weirder, when it tries to download the objects.inv from python.org it just hangs. If I manually cd to the build directory for gappy and run Are we actually doing something to block the internet connection when installing packages? |
|
comment:8
It eventually fails with |
|
comment:9
A simple workaround suggested here is to write a wrapper around the original |
|
comment:11
Yep, this approach gets the job done. In case we also have a local copy of the Python docs installed, this can also change the intersphinx mapping to generate links to the local docs instead of the online ones, if desired. Same approach could be used in other packages (pplpy, etc.) Next step: add the intersphinx mapping to the Sage docs, again generating links to the local gappy docs if available, and otherwise to the online docs. |
|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
|
|
comment:13
This is nearly ready to go from my point of view, except that when I run the full test suite I get a weird segfault from the Singular interface around here: which seems to be emanating from I'm not sure what this would possibly have to do with the libgap interface, but it's not outside the realm of possibility. When I re-run the failing test by itself the segfault does not occur :/ |
|
comment:14
Should #31297 be listed as a dependency? |
|
comment:16
Thanks for the clarification. I have set the milestone of #31297 accordingly. |
|
comment:17
I fought to make initialization of @@ -581,31 +576,11 @@ cdef class PermutationGroupElement(MultiplicativeGroupElement):
...
ValueError: invalid data to initialize a permutation
"""
- cdef UInt2* p2
- cdef UInt4* p4
- cdef int i
- cdef UInt d
-
- if TNUM_OBJ(p.value) == T_PERM2:
- d = DEG_PERM2(p.value)
- if d > self.n:
- d = self.n
- else:
- for i in range(d, self.n):
- self.perm[i] = i
- p2 = CONST_ADDR_PERM2(p.value)
- for i in range(d):
- self.perm[i] = p2[i]
- elif TNUM_OBJ(p.value) == T_PERM4:
- d = DEG_PERM4(p.value)
- if d > self.n:
- d = self.n
- else:
- for i in range(d, self.n):
- self.perm[i] = i
- p4 = CONST_ADDR_PERM4(p.value)
- for i in range(d):
- self.perm[i] = p4[i]
+ if isinstance(p, GapPermutation):
+ try:
+ self._set_list_images(p.ListPerm(), False)
+ except AssertionError:
+ raise ValueError("invalid data to initialize a permutation")
else:
raise TypeError("not a gap permutation")Is there any reason why not use the C API of GAP here? |
|
comment:18
Replying to @videlec:
Yes, the goal is to do away as much as possible with using undocumented/potentially unstable GAP internals. The GAP community has shown an interest of late of developing something more of a "public" API for using GAP as a library, and I and others from the Sage side have been working with them a lot to develop that. Unfortunately, one of the "gaps" is public APIs for accessing permutation objects efficiently. I'm glad you brought this up though because I was worried about exactly this change, and was not 100% sure how impactful it was. Is there an example case you can give me that might show whether or not there is a performance deficit? I've made other performance improvements that might negate the change, but you are right that when using |
|
comment:19
I'll add, if it makes an enormous difference, I can make an exception for now. It wouldn't be the only exception I've made in the current version of gappy for APIs that are not yet available in the libgap API. I'll propose some additions to libgap for accessing permutations, and then in gappy provide wrappers that provide the currently missing APIs. |
|
comment:20
Some more info on the failing Singular test: The test in question is testing an alarm interrupt, so it seems maybe the interrupt is occurring before this A couple weird things about this:
|
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
comment:47
libgap workspace management is still broken, as can be seen by removing |
|
comment:48
I see how the workspace issue can be fixed, but it might need a change in |
|
Upstream: Reported upstream. No feedback yet. |
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
Changed reviewer from Dima Pasechnik to Dima Pasechnik, ... |
|
comment:50
we still need to carefully check that GAP workspace rotation is not broken, but hopefully not. |
|
comment:51
a couple of test failures I can't quite make sense of. The 1st one has something to do with The 2nd one is even less explicit, with a mysterious |
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
comment:53
I still wouldn't move forward on this too much until I have a first release of gappy out. I haven't had time to work on it in several months, so it's probably fallen a bit behind the last GAP releases (and can probably drop some workarounds for older GAP versions). Other than that there isn't too much holding up making a gappy release--I think, as you found, some of the workspace management stuff, and some of the global namespace stuff from Sage's libgap needs to be worked into it better. |
|
comment:54
Stalled in |
|
comment:55
See also: #33072. |
This comment has been minimized.
This comment has been minimized.
|
comment:57
Moving to "needs work," as there's nothing to review at the moment (we're waiting for an upstream gappy release). |
|
comment:58
Upstream changed jobs recently, not sure if an update will be soon... |
From the gappy README:
This is a follow-up to #31297, particularly inspired by #31297 comment:17. It is a more disruptive change, in that instead of providing wrappers around gappy that are Sage Parents and Elements, it follows the example of cypari2 and just uses gappy more-or-less directly without any wrappers.
It remains completely agnostic to the coercion system, though I am not completely happy with this state of affairs. In particular you can see I had to add a special case to
Polynomial.__call__for handling evaluating polynomials onGapObjs, a case that used to work fine, but now needs a special case since other Sage types cannot be coerced toGapObjs.Also had to add special cases for instantiating
Integers andRationals fromGapObjs, but on the plus side this is now a bit faster.Upstream: Reported upstream. No feedback yet.
CC: @videlec @dimpase @mkoeppe @slel
Component: interfaces
Keywords: gap libgap gappy
Work Issues: release gappy 0.1.0 final and update spkg version before merging, test against p_groups_cohomology
Author: Erik Bray
Branch/Commit: u/dimpase/gappy-without-wrappers @
3f1d051Reviewer: Dima Pasechnik, ...
Issue created by migration from https://trac.sagemath.org/ticket/31404
The text was updated successfully, but these errors were encountered: