Opened 13 years ago

Closed 9 years ago

#5489 closed enhancement (fixed)

Add an interface for 4ti2 to Sage

Reported by: mhansen Owned by: was
Priority: major Milestone: sage-5.4
Component: interfaces Keywords:
Cc: Merged in: sage-5.4.beta0
Authors: Mike Hansen, Bjarke Hammersholt Roune Reviewers: Martin Albrecht, John Palmieri, Marshall Hampton, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Adds an interface to the (currently optional) 4ti2 package.

4ti2 is necessary for full functionality of the sandpiles module. In the near future it would be nice to have 4ti2 as a standard component of Sage.

Apply only: trac_5489_4ti2_interface-reviewer.patch

Attachments (4)

4ti2.sage (6.3 KB) - added by mhansen 13 years ago.
4ti2_interface.patch (17.0 KB) - added by broune 13 years ago.
trac_5489_4ti2_interface.patch (17.9 KB) - added by mhampton 11 years ago.
Cumulative patch
trac_5489_4ti2_interface-reviewer.patch (17.4 KB) - added by chapoton 9 years ago.

Download all attachments as: .zip

Change History (22)

Changed 13 years ago by mhansen

Changed 13 years ago by broune

comment:1 Changed 13 years ago by broune

  • Summary changed from add a non library level interface to 4ti2 to Sage to [with patch, needs review] Add an interface for 4ti2 to Sage

The patch 4ti2_interface.patch builds on mhansens work. It adds the method groebner, which computes Toric grobner bases, and adds documentation and doctests. It also updates the code so that it can be built as part of Sage.

You need 4ti2 installed to review this, but the experimental spkg from 2006 is incompatible with this patch. Download 4ti2 from www.4ti2.de and put the binaries from that somewhere on your Sage path. E.g. dumping them in sage/local/bin will work.

Patch applies cleanly to Sage 4.0.1 and doctests pass on my Mac.

comment:2 Changed 12 years ago by malb

  • Summary changed from [with patch, needs review] Add an interface for 4ti2 to Sage to [with patch, needs work] Add an interface for 4ti2 to Sage

Review

  • the docstrings are not according to the current Sage standard.
  • IMHO one should update the experimental 4ti2 spkg in parallel with accepting this patch
  • how much hassle would it be to replace docs like
    Runs the 4ti2 program ``qsolve`` on the parameters. See ``http://www.4ti2.de/`` for details. 
    
    with docs which describe the program somewhat?
  • patch applies cleanly against 4.1.1.
  • devel/sage/sage/interfaces/four_ti_2.py # 9 doctests failed if 4ti2 is not installed, because #optional tag is missing

comment:3 follow-up: Changed 12 years ago by broune

Thank you for the review. Could you tell me what needs to change in the docstring format? I'm willing to fix this, as well as add the #optional tags, if that is all that is needed for acceptance of the patch.

Inlining the 4ti2 documentation from 4ti2 into Sage opens the issue of licenses. I do not know what license is used for the 4ti2 documentation, and I don't think this is explicit anywhere. I agree it would be an improvement, though this would have to wait until someone (not me) in future volunteers to investigate the 4ti2 license for documentation.

I agree it would be good to update the experimental 4ti2 spkg, which is however beyond the scope of what I'm willing to do on this at this time. If this is a requirement of acceptance of the patch, it will have to wait until someone chooses to update the 4ti2 spkg.

comment:4 in reply to: ↑ 3 Changed 12 years ago by jhpalmieri

Replying to broune:

Thank you for the review. Could you tell me what needs to change in the docstring format? I'm willing to fix this, as well as add the #optional tags, if that is all that is needed for acceptance of the patch.

I don't have answers for your other questions, but for this one:

(1) "EXAMPLES:" should be changed to "EXAMPLES::" (double colon), and it should be followed by a blank line.

(2) In the __init__ method at the beginning of the file, the INPUT block is not formatted correctly: after the first line, the other lines should be indented so that they line up with ``directory`` (as you've done later).

(3) In all of your INPUT blocks, the leading hyphens should not be indented: they should line up with the "I" in "INPUT".

(4) A few methods have blank lines after the initial r""". I think those should be deleted.

comment:5 Changed 11 years ago by mhampton

  • Description modified (diff)
  • Report Upstream set to N/A
  • Status changed from needs_work to needs_review

comment:6 Changed 11 years ago by mhampton

I believe I have addressed all the comments of jhpalmieri above. It is true that better descriptions of the functionality would be nice, but I think this is OK for inclusion anyway - sometimes the perfect is the enemy of the good.

Although it does not have a positive review (please someone take a look!) please test this against http://trac.sagemath.org/sage_trac/ticket/8217.

Changed 11 years ago by mhampton

Cumulative patch

comment:7 Changed 10 years ago by davidloeffler

  • Summary changed from [with patch, needs work] Add an interface for 4ti2 to Sage to Add an interface for 4ti2 to Sage

comment:8 Changed 10 years ago by davidloeffler

Apply trac_5489_4ti2_interface.patch

(for the patchbot)

comment:9 Changed 10 years ago by chapoton

The code looks good, the doc coverage is 100% and the tests pass. I am almost ready to give a positive review.

I have only some basic comments:

  • there is a typo here in "does" (in the method directory)
# method since apparently importing sage.misc.misc oes not
  • Why not gather these import statements at the beginning ?
 from sage.matrix.constructor import matrix 
 from sage.matrix.matrix import is_Matrix 
 from sage.rings.integer_ring import ZZ 
 import subprocess

If there is no technical difficulty (as for sage.misc.misc), it seems better to import them once and for all. At least those about matrices and integers ?

  • There seem to be some 'trailing whitespaces' that could be removed. All the lines just after an EXAMPLES:: should rather be empty.
  • The minimize method is NotImplemented?. This can of course wait for another ticket.

comment:10 Changed 9 years ago by jdemeyer

Please fill in your real name as Author.

Changed 9 years ago by chapoton

comment:11 Changed 9 years ago by chapoton

  • Description modified (diff)

comment:12 Changed 9 years ago by chapoton

Apply only: trac_5489_4ti2_interface-reviewer.patch

comment:13 Changed 9 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

I have made some small changes that I suggested, in a new patch. The bot is green, and everything looks good. Positive review !

comment:14 Changed 9 years ago by mhampton

  • Authors set to Mike Hansen, Bjarke Hammersholt Roune
  • Reviewers changed from Frédéric Chapoton to Marshall Hampton, Frédéric Chapoton

comment:15 Changed 9 years ago by mhampton

  • Reviewers changed from Marshall Hampton, Frédéric Chapoton to Martin Albrecht, John Palmieri, Marshall Hampton, Frédéric Chapoton

comment:16 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.3 to sage-5.4

comment:17 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:18 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.4.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.