# HG changeset patch
# Parent e9ad0f9ac557c4f63fc9526672de55096634935a
Some old pickles of some users could not be unpickled, since gap was used to store important data.
Fortunately, the missing information can be easily reconstructed from other data. Also, it is made
sure that this data will not be using GAP, even if the user gives the SmallGroups address by GAP
integers rather than by Sage integers.

diff --git a/src/pGroupCohomology/cohomology.pyx b/src/pGroupCohomology/cohomology.pyx
--- a/src/pGroupCohomology/cohomology.pyx
+++ b/src/pGroupCohomology/cohomology.pyx
@@ -141,9 +141,16 @@
         
     ## Now, StateFile should point to a file providing the state of the cohomology ring.
     ## If it doesn't, then we need to find out *after* loading
-
+    second_cache_attempt = False
     if not OPTION.opts.has_key('@newroot@'):
-        OUT = _cache.get((GroupKey,StateFile)) or _cache.get((GroupKey,StateFile[:-5]))
+        try:
+            OUT = _cache.get((GroupKey,StateFile)) or _cache.get((GroupKey,StateFile[:-5]))
+        except StandardError, msg:
+            print_protocol("WARNING: %s"%msg)
+            print_protocol("         The given group key seems to contain invalid data.")
+            print_protocol("         Will try to recover later.")
+            OUT = None
+            second_cache_attempt = True
         if OUT is not None:
             ## test if the singular subprocess is still running
             # does not really seem necessary
@@ -163,6 +170,12 @@
         print_protocol("WARNING: Files on disk have been moved or are not writeable.")
         print_protocol("         Will try to recover later.")
         OUT.GStem = original_GStem
+        print_protocol("Trying to reconstruct cache key from %s"%original_GStem)
+        try:
+            q,n = [Integer(nb) for nb in original_GStem.split('gp')]
+            OUT.setprop('_key', ((q,n),StateFile))
+        except StandardError:
+            print_protocol("         Group identifier not reconstructible from %s."%original_GStem)
         OUT.setprop('_need_new_root', OPTION.opts.get('@use_this_root@',True))
         return OUT
     try:
@@ -170,16 +183,30 @@
             ST = load(StateFile)  # realpath here?
         else:
             ST = load(StateFile+'.sobj')  # realpath here?
-    except (OSError, IOError):
+    except (OSError, IOError),msg:
         print_protocol("WARNING: Files on disk have been moved or are not readable.")
         print_protocol("         Will try to recover later.")
         OUT.setprop('_need_new_root', OPTION.opts.get('@use_this_root@',True))
         if OPTION.opts.has_key('@use_this_root@'):
             OPTION.opts.__delitem__('@use_this_root@')
+        print_protocol("Trying to reconstruct cache key from %s"%original_GStem)
+        try:
+            q,n = [Integer(nb) for nb in original_GStem.split('gp')]
+            OUT.setprop('_key', ((q,n),StateFile))
+        except StandardError:
+            print_protocol("         Group identifier not reconstructible from %s."%original_GStem)
+        if second_cache_attempt:
+            NewOUT = _cache.get(OUT._key)
+            if NewOUT is not None:
+                return NewOUT
         return OUT
     # Apparently the root given by StateFile is correct. So, use it,
     # regardless what the contents of the file pointed at by StateFile say!
     OUT.__setstate__(ST, newroot=OPTION.opts.get('@use_this_root@') or root)
+    if second_cache_attempt:
+        NewOUT = _cache.get(OUT._key)
+        if NewOUT is not None:
+            return NewOUT
     _cache[GroupKey, StateFile] = OUT
     if OPTION.opts.has_key('@use_this_root@'):
         OPTION.opts.__delitem__('@use_this_root@')
@@ -284,7 +311,7 @@
     except:
         return G
     if isinstance(G,dict):
-        return dict((unpickle_gap_data(k, gap),v) for k,v in G.iteritems())
+        return dict((unpickle_gap_data(k, gap), unpickle_gap_data(v, gap)) for k,v in G.iteritems())
     return type(G)(unpickle_gap_data(X, gap) for X in I)
 
 
@@ -310,7 +337,7 @@
 
         sage: from pGroupCohomology.cohomology import GapPickler, unpickle_gap_data, pickle_gap_data
         sage: G = gap.SmallGroup(8,3).IsomorphismPermGroup().Image()
-        sage: D = {(1, G, "abc"):5}
+        sage: D = {(1, G, "abc"):(5,G)}
         sage: unpickle_gap_data(pickle_gap_data(D), gap) == D
         True
 
@@ -339,7 +366,7 @@
     except:
         return G
     if isinstance(G,dict):
-        return dict((pickle_gap_data(k),v) for k,v in G.iteritems())
+        return dict((pickle_gap_data(k), pickle_gap_data(v)) for k,v in G.iteritems())
     return type(G)(pickle_gap_data(X) for X in I)
 
 #########################################################
@@ -2625,7 +2652,7 @@
         else:
             from pGroupCohomology.resolution import gap
         from pGroupCohomology.resolution import _gap_init
-        _gap_init(gap) #gap.RestoreStateRandom(0)
+        _gap_init(gap)
         if len(args) == 2:
             # We expect an address in the Small Groups library
             q,n = args
@@ -2712,7 +2739,6 @@
                     Hfinal = H0
                 else:
                     print_protocol("Try to find minimal generators for %s"%repr(H0))
-                    #GAP.eval('Reset(GlobalMersenneTwister,0)') #GAP.RestoreStateRandom(0)
                     HP = GAP('Group(Image(IsomorphismPermGroup(%s),GeneratorsOfGroup(%s)))'%(H0.name(),H0.name()))
                     ## Hopefully gap can find minimal generating sets for permutation groups...
                     Hfinal = GAP('Subgroup(%s,MinimalGeneratingSet(%s))'%(HP.name(),HP.name()))
@@ -2723,7 +2749,6 @@
                     GroupKey = kwds['key'][0]
                 else:
                     if not GAP('IsPermGroup(%s)'%(Hfinal.name())):
-                        #GAP.eval('Reset(GlobalMersenneTwister,0)') #GAP.RestoreStateRandom(0)
                         GroupKey = repr(GAP('regularPermutationAction(%s: forceDefiningGenerators)'%Hfinal.name())) # GAP.eval('Group(Image(IsomorphismPermGroup(%s),GeneratorsOfGroup(%s)))'%(Hfinal.name(),Hfinal.name()))
                     else:
                         GroupKey = 'Group('+repr(Hfinal.GeneratorsOfGroup())+')'
@@ -3218,7 +3243,6 @@
             StateFile = os.path.join('@public_db@',self.GStem,'dat','State')
         else:
             StateFile = self._key[1]
-        #print StateFile
         return COHO_unpickle, (self._key[0], StateFile)
 
     def __getstate__(self):
@@ -3472,7 +3496,13 @@
             ## First, the "Additional Properties" are reconstructed
             for X,Y in Dict:
                 self._property_dict[X]=Y
-            self._decorator_cache = dict([(unpickle_gap_data(k, gap),v) for k,v in cache])
+            self._decorator_cache = tmp_dict = dict()
+            for k,v in cache:
+                try:
+                    tmp_dict[unpickle_gap_data(k, gap)] = unpickle_gap_data(v, gap)
+                except StandardError,msg:
+                    print "WARNING",msg
+                    print "Unable to reconstruct some data in GAP"
 
             ##########
             ## Try to find the folder in which the cohomology data are rooted.
@@ -3601,11 +3631,19 @@
             ## The key for uniqueness of parent structures
             if self._key is not None: # we will explicitly provide the second half of the key (file location),
                                       # but will try to re-use the first half (group description)
-                self.setprop('_key', (self._key[0], os.path.join(root,dat_folder,'State')))
+                # Problem: It could be that the old key was not created properly, e.g.,
+                # by referring to a GAP session. If this is the case, we need to reset it.
+                try:
+                    h = hash(self._key[0])
+                    self.setprop('_key', (self._key[0], os.path.join(root,dat_folder,'State')))
+                except StandardError,msg:
+                    print_protocol("WARNING: %s"%msg)
+                    print_protocol("         Stored cache key was not readable")
+                    print_protocol("         Will try to reconstruct it from the group identifier %s"%GStem)
+                    self.delprop('_key')
             if self._key is None:
                 try:
                     q,n = [Integer(nb) for nb in GStem.split('gp')]
-                    #print 'Found', (q,n)
                     self.setprop('_key', ((q,n),os.path.join(self.dat_folder,'State')))
                 except (TypeError, ValueError):
                     print 'WARNING: No good group key found'
@@ -4294,9 +4332,6 @@
                 pass
         from pGroupCohomology.resolution import gap, _gap_init
         _gap_init(gap)
-        #gap.eval('Read("%s/local/pGroupCohomology/GapMaxels");'%(SAGE_ROOT))
-        #gap.eval('Read("%s/local/pGroupCohomology/GapMB");'%(SAGE_ROOT))
-        #gap.eval('Read("%s/local/pGroupCohomology/GapSgs");'%(SAGE_ROOT))
         if isinstance(self._key[0], basestring):
             self._gap_group = gap(self._key[0])
         elif len(self._key[0])==1:
@@ -4500,14 +4535,13 @@
         # returned. Hence, when working with it, very soon this __getattr__ method
         # will be called. At this point, _default_filename might tell us where the
         # file and hence the ring data are located!
-        #print 'getattr',key
         import os
         if key == '_default_filename': # This ought to be a *proper* attribute, no fake attribute!
             raise AttributeError, "'pGroupCohomology.cohomology.COHO' object has no attribute '_default_filename'"
         if key == '__members__':
             return self._property_dict.keys()
         if self._property_dict.get('_need_new_root'):
-            print_protocol('Files on disk have been moved - trying to get things right',self)
+            print_protocol('Files on disk have been moved - trying to get things right', self.GStem)
             if isinstance(self._property_dict['_need_new_root'],basestring):
                 newroot = self._property_dict['_need_new_root']
                 defaultname = os.path.join(newroot,self.GStem,'H'+self.GStem+'.sobj')
@@ -10839,12 +10873,10 @@
         # the non-quotiented groups
         for i from l > i > 1:
             q,n = gap('IdGroup(%s[%d])'%(L.name(),i)).sage()
-            #gap.eval('Reset(GlobalMersenneTwister,0)') #gap.RestoreStateRandom(0)
             G[-i+1] = (gap('IsomorphismGroups(%s[%d], SmallGroup(%d,%d))'%(L.name(),i,q,n)), [q,n]) # just choose one isomorphism
         
         # Unfortunately, Gap allows itself to change the generators of self.group() when computing L[1].
         # We need to undo it:
-        #gap.eval('Reset(GlobalMersenneTwister,0)') #gap.RestoreStateRandom(0)
         G[0] = (gap('IsomorphismGroups(%s[1],%s)'%(L.name(),self.group().name())),[0,0])
 
         # the factor groups
@@ -10852,7 +10884,6 @@
         for i from 1 <= i < l-1:
             QuotMap[i] = gap('NaturalHomomorphismByNormalSubgroup(%s[1],%s[%d])'%(L.name(),L.name(),l-i))
             q,n = gap('IdGroup(Image(%s))'%(QuotMap[i].name())).sage()
-            #gap.eval('Reset(GlobalMersenneTwister,0)') #gap.RestoreStateRandom(0)
             G[i] = (gap('IsomorphismGroups(Image(%s),SmallGroup(%d,%d))'%(QuotMap[i].name(),q,n)), [q,n])
             gap.eval('%s := GroupHomomorphismByImages(Range(%s),Range(%s), GeneratorsOfGroup(Image(%s)), List([1..Length(GeneratorsOfGroup(Image(%s)))], x -> Image(%s, Image(%s, PreImagesRepresentative(%s, GeneratorsOfGroup(Image(%s))[x])))))'%(QuotMap[i].name(), G[0][0].name(),G[i][0].name(), G[0][0].name(), G[0][0].name(), G[i][0].name(),QuotMap[i].name(),G[0][0].name(), G[0][0].name()))
         
@@ -12084,7 +12115,6 @@
             for NR, chm in self.RestrMaps.items():
                 print_protocol("Restriction to %s subgroup (order %s)"%(Ordinals(NR),chm[0][0]), self)
                 while(chm[1].knownDeg()<n):
-                    #print 'try and lift', chm
                     chm[1].lift()
                 if OPTION.opts['sparse']:
                     import os
@@ -12138,7 +12168,6 @@
                     print_protocol("> There is 1 nilpotent generator in degree %d"%( n), self)
                 else:
                     print_protocol("> There are %d nilpotent generators in degree %d"%(len(NewGen),n), self)
-            #print 'Nil', NewGen
 
         ###########
         ## 2. New generators with nilpotent restriction on the
diff --git a/src/pGroupCohomology/factory.py b/src/pGroupCohomology/factory.py
--- a/src/pGroupCohomology/factory.py
+++ b/src/pGroupCohomology/factory.py
@@ -672,13 +672,26 @@
             sage: CohomologyRing.create_group_key([gap('SymmetricGroup(8)')],GroupId=[20,2])
             (20, 2)
 
+        TEST:
+
+        It is important that the group key is not formed by two integers in
+        the GAP interface. Namely, when storing the resulting ring, it could
+        not easily been unpickled (actually it *can* be unpickled, but this
+        involves some trickery, and it is certainly better to not rely on
+        trickery). Here, we demonstrate that the given keys are correctly converted::
+
+            sage: CohomologyRing.set_user_db(tmp_dir())
+            sage: X = CohomologyRing(gap(8),gap(3), from_scratch=True)
+            sage: type(X._key[0][0])
+            <type 'sage.rings.integer.Integer'>
+
         """
         if GroupDefinition:
             return GroupDefinition
         if len(G)==2:
-            return (G[0],G[1])
+            return (Integer(G[0]),Integer(G[1]))
         if GroupId:
-            return (GroupId[0],GroupId[1])
+            return (Integer(GroupId[0]),Integer(GroupId[1]))
         # Try to determine a key from GAP
         g = G[0]
         if not (hasattr(g,'parent') and repr(g.parent())=='Gap'):
@@ -689,7 +702,7 @@
             gId = g.IdGroup().sage()
             gs = gap.SmallGroup(gId)
             if gap.eval('canonicalIsomorphism(%s,%s)'%(g.name(),gs.name()))!='fail':
-                return gId[0],gId[1]
+                return Integer(gId[0]),Integer(gId[1])
         except (RuntimeError,TypeError):
             pass
         if g.IsPermGroup():
