add Carlo Hamalainen's latin square stuff to Sage

mwhansen commented 16 years ago

CC: @sagetrac-sage-combinat

Component: combinatorics

Issue created by migration from https://trac.sagemath.org/ticket/1707

wdjoyner commented 16 years ago

comment:3

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.

wdjoyner commented 16 years ago

comment:4

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).

b6699e46-0a76-473a-9c8a-88795bfcd32d commented 16 years ago

Dancing links C++

b6699e46-0a76-473a-9c8a-88795bfcd32d commented 16 years ago

Attachment: dancing_links.cpp.gz

Attachment: dancing_links.spyx.gz

Dancing links C++ wrapper

b6699e46-0a76-473a-9c8a-88795bfcd32d commented 16 years ago

Updated latin squares code (lots of doctests), replaces latin.sage

b6699e46-0a76-473a-9c8a-88795bfcd32d commented 16 years ago

comment:5

Attachment: latin.2.sage.gz

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)

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 16 years ago

comment:6

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

wdjoyner commented 16 years ago

comment:7

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?

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 16 years ago

comment:8

Replying to @wdjoyner:

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

b6699e46-0a76-473a-9c8a-88795bfcd32d commented 16 years ago

Attachment: trac1707-combinat-matrices.patch.gz

patch against 2.11.alpha1, needs review

wdjoyner commented 16 years ago

comment:10

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

mwhansen commented 16 years ago

comment:11

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

mwhansen commented 16 years ago

comment:12

Attachment: 1707-referee.patch.gz

Apply the last two patches: trac1707-combinat-matrices.patch and 1707-referee.patch

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 16 years ago

comment:13

Merged trac1707-combinat-matrices.patch and 1707-referee.patch in Sage 2.11.alpha2