#1707 closed enhancement (fixed)
[with patch, positive review] add Carlo Hamalainen's latin square stuff to Sage
Reported by: | mhansen | Owned by: | mhansen |
---|---|---|---|
Priority: | minor | Milestone: | sage-2.11 |
Component: | combinatorics | Keywords: | |
Cc: | sage-combinat | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Attachments (7)
Change History (22)
comment:1 Changed 13 years ago by
- Component changed from algebraic geometry to combinatorics
- Milestone set to sage-2.10
- Owner changed from was to mhansen
- Priority changed from major to minor
- Status changed from new to assigned
- Summary changed from add Carlo Hamalainen latin square stuff to Sage to add Carlo Hamalainen's latin square stuff to Sage
- Type changed from defect to enhancement
comment:2 Changed 13 years ago by
- Summary changed from add Carlo Hamalainen's latin square stuff to Sage to [with code, needs review] add Carlo Hamalainen's latin square stuff to Sage
comment:3 Changed 13 years ago by
comment:4 Changed 13 years ago by
I've looked at this more. This is not a patch at all but simply some SAGE code. A lot of this seems like good code but a lot is missing for this to be acceptable for inclusion in SAGE - missing docstrings and doctests, and I think some functions should be methods for a (yet to be created and designed) LatinSquare? class. Suggestion: This could all go into a subdirectory of combinat called matrices, since there is a wide subfield of combinatorics which deals with matrices of various types (eg, latin squares, Hadamard matrices, (0,1)-matrices, etc).
comment:5 Changed 13 years ago by
- The file dfs_latin.spyx can be removed, all functionality has been superseded by the C++ dancing links solver.
- Many doctests/docstrings added to latin.sage (see latin.2.sage attachment)
comment:6 Changed 13 years ago by
- Milestone changed from sage-3.0 to sage-2.11
Hi,
I have deleted latin.sage and dfs_latin.spyx. As David did state earlier we now need to rename the files, add them to the build systems, add imports and finally turn this into a patch. Then hopefully somebody will review this quickly.
Anybody around who wants to help Carlo out?
Cheers,
Michael
comment:7 follow-up: ↓ 8 Changed 13 years ago by
I can try to help in the beginning. However, I've forgotten how to add a new directory to the devel tree. Do you use hg_sage.add as well?
comment:8 in reply to: ↑ 7 Changed 13 years ago by
Replying to wdj:
I can try to help in the beginning. However, I've forgotten how to add a new directory to the devel tree. Do you use hg_sage.add as well?
To the Sage repo? Just add it and add the new file to the repo. Somebody should add this to the development manual in case it isn't in there yet, i.e. "How do I add my code to the tree in case it is all new".
Cheers,
Michael
comment:9 Changed 13 years ago by
- Summary changed from [with code, needs review] add Carlo Hamalainen's latin square stuff to Sage to [with patch, needs review] add Carlo Hamalainen's latin square stuff to Sage
comment:10 Changed 13 years ago by
- Summary changed from [with patch, needs review] add Carlo Hamalainen's latin square stuff to Sage to [with patch, positive review] add Carlo Hamalainen's latin square stuff to Sage
This applies cleanly, but not not build without an addition to setup.py (in the top directory), since it adds a subdirectory "matrices" to combinat. With this change, it passes sage -testall, except for the plot.py failure (which has nothing to do with this patch).
I give this a positive review but leave some food for thought: In my opinion, at some point, possibly in a future version, some very minor changes to the docstring are worth considering:
- "nauty?" should be replaced by "NICE?"
- add REFERENCES section, with actual literature sources
- allow base rings other than ZZ
- discuss whether isotopism sould return a Permutation or a
PermutationGroupElement? with sage-devel
comment:11 Changed 13 years ago by
- Summary changed from [with patch, positive review] add Carlo Hamalainen's latin square stuff to Sage to [with patch, needs review] add Carlo Hamalainen's latin square stuff to Sage
Hello,
I'll post a more in-depth review in awhile, but one thing that needs to be done is to make it so that doctests pass.
1) 'sage.combinat.matrices' has to be added in setup.py under 'sage.combinat.sf' (for example) so that Sage knows about the module.
2) All the doctests for functions which are not raised to the global namespace (via matrices.all) need to be explicitly imported in the doctest. For example, "sage: from sage.combinat.matrices.latin import dlxcpp_find_completions".
--Mike
Changed 13 years ago by
comment:12 Changed 13 years ago by
- Summary changed from [with patch, needs review] add Carlo Hamalainen's latin square stuff to Sage to [with patch, positive review] add Carlo Hamalainen's latin square stuff to Sage
Apply the last two patches: trac1707-combinat-matrices.patch and 1707-referee.patch
comment:13 Changed 13 years ago by
- Resolution set to fixed
- Status changed from assigned to closed
Merged trac1707-combinat-matrices.patch and 1707-referee.patch in Sage 2.11.alpha2
Changed 13 years ago by
comment:14 Changed 13 years ago by
Merged trac_1707_pbuild.patch and trac_1707-dancing_links.pyx-doctestfix.patch in Sage 2.11.alpha2
Cheers,
Michael
comment:15 Changed 12 years ago by
- Cc sage-combinat added
I'm willing to review this but since the patch was created 2 months ago(?), I am not sure what version of SAGe to apply it against. Looks very interesting.