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: |
Description (last modified by )
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)
Change History (22)
Changed 13 years ago by
Changed 13 years ago by
comment:1 Changed 13 years ago by
- 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
comment:2 Changed 12 years ago by
- 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: ↓ 4 Changed 12 years ago by
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
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
- Description modified (diff)
- Report Upstream set to N/A
- Status changed from needs_work to needs_review
comment:6 Changed 11 years ago by
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.
comment:7 Changed 10 years ago by
- 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
Apply trac_5489_4ti2_interface.patch
(for the patchbot)
comment:9 Changed 10 years ago by
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
Please fill in your real name as Author.
Changed 9 years ago by
comment:11 Changed 9 years ago by
- Description modified (diff)
comment:12 Changed 9 years ago by
Apply only: trac_5489_4ti2_interface-reviewer.patch
comment:13 Changed 9 years ago by
- 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
- Reviewers changed from Frédéric Chapoton to Marshall Hampton, Frédéric Chapoton
comment:15 Changed 9 years ago by
- 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
- Milestone changed from sage-5.3 to sage-5.4
comment:17 Changed 9 years ago by
- Description modified (diff)
comment:18 Changed 9 years ago by
- Merged in set to sage-5.4.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
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.