Ticket #2171 (closed defect: fixed)

Opened 5 years ago

Last modified 5 years ago

[with patch; positive review] followup to #2169 -- (magma/sage interface) some further optimizations and fixes

Reported by: was Owned by: was
Priority: major Milestone: sage-3.2
Component: interfaces Keywords: editor_craigcitro
Cc: Work issues:
Report Upstream: Reviewers:
Authors: Merged in:
Dependencies: Stopgaps:

Description

Apply the patches from #2169, then apply both these patches. To test do

sage -t --optional SAGE_ROOT/devel/sage/sage/interfaces/magma.py

Conversion of Magma matrices over ZZ back to Sage should also be much faster now.

Attachments

extcode-trac2171.patch Download (2.5 KB) - added by was 5 years ago.
extcode-trac2171-part2.patch Download (1.7 KB) - added by was 5 years ago.
sage-trac2171-part1.patch Download (3.4 KB) - added by was 5 years ago.
sage-trac2171-part2.patch Download (2.6 KB) - added by was 5 years ago.

Change History

Changed 5 years ago by was

Changed 5 years ago by was

comment:1 Changed 5 years ago by was

Apply all the attached files -- the sage- ones to hg_sage and the extcode ones to hg_extcode.

comment:2 Changed 5 years ago by mhansen

I get a reject with  http://sagetrac.org/sage_trac/attachment/ticket/2171/sage-trac2171.patch on rc1. It looks like it is depending on a patch that isn't there. This is the failure:

--- expect.py
+++ expect.py
@@ -860,10 +860,15 @@ If this all works, you can then make cal
         return self.eval(var)
 
     def get_using_file(self, var):
-        """
+        r"""
         Return the string representation of the variable var in self
         using a file.  Use this if var has a huge string
         representation.  It'll be way faster.
+
+        WARNING: In fact unless a special derived class implements
+        this, it will \emph{not} be any faster.  This is the case for
+        this class if you're reading it through introspection and
+        seeing this.
         """
         return self.get(var)

and this is expect.py in rc1:

    def get_using_file(self, var):
        return self.get(var)

comment:3 Changed 5 years ago by was

  • Milestone changed from sage-2.11 to sage-2.10.4

comment:4 Changed 5 years ago by craigcitro

The patch that this code depends on is attached to trac #2120. I guess this will have to wait until that patch is ready to go? Or should we pull over that part of the patch to this ticket so we can get it merged? William, what's the status of the Maple ticket?

comment:5 Changed 5 years ago by craigcitro

  • Keywords editor_craigcitro added

comment:6 Changed 5 years ago by craigcitro

  • Summary changed from [with patch; needs review] followup to #2169 -- (magma/sage interface) some further optimizations and fixes to [with patch; waiting on #2120 before review] followup to #2169 -- (magma/sage interface) some further optimizations and fixes

comment:7 Changed 5 years ago by mabshoff

Craig,

can you review this? It has been potentially bitrotting for a long, long time :)

Cheers,

Michael

Changed 5 years ago by was

Changed 5 years ago by was

comment:8 Changed 5 years ago by was

  • Summary changed from [with patch; waiting on #2120 before review] followup to #2169 -- (magma/sage interface) some further optimizations and fixes to [with patch; needs review] followup to #2169 -- (magma/sage interface) some further optimizations and fixes

I rebased these patches against 3.2.alpha0 and got rid of the dependence on #2120. This should be easy to apply and go into sage-3.2.

comment:9 Changed 5 years ago by mabshoff

  • Milestone changed from sage-3.2.1 to sage-3.2

In sage-trac2171-part1.patch there is still a change to sage/interfaces/maple.py. I will delete that hunk and test the patch. Other than that I expect a positive review assuming the doctests pass.

Cheers,

Michael

comment:10 Changed 5 years ago by mvngu

For the patch extcode-trac2171-part2.patch, here's a possible documentation fix:

-{Conver the ring of integers to Sage.}
+{Convert the ring of integers to Sage.}

comment:11 Changed 5 years ago by mabshoff

Positive review.

Cheers,

Michael

comment:12 Changed 5 years ago by mabshoff

  • Status changed from new to closed
  • Resolution set to fixed

Merged sage-trac2171-part1.patch and sage-trac2171-part2.patch in Sage 3.2.alpha1

comment:13 Changed 5 years ago by mabshoff

  • Summary changed from [with patch; needs review] followup to #2169 -- (magma/sage interface) some further optimizations and fixes to [with patch; positive review] followup to #2169 -- (magma/sage interface) some further optimizations and fixes

Oops, I didn't merge extcode-trac2171.patch and extcode-trac2171-part2.patch. Those two patches have been merged in Sage 3.2.alpha2.

Cheers,

Michael

Note: See TracTickets for help on using tickets.