Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#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:

Status badges

Description


Attachments (7)

dancing_links.cpp (9.0 KB) - added by carlohamalainen 13 years ago.
Dancing links C++
dancing_links.spyx (2.5 KB) - added by carlohamalainen 13 years ago.
Dancing links C++ wrapper
latin.2.sage (54.2 KB) - added by carlohamalainen 13 years ago.
Updated latin squares code (lots of doctests), replaces latin.sage
trac1707-combinat-matrices.patch (86.7 KB) - added by carlohamalainen 13 years ago.
patch against 2.11.alpha1, needs review
1707-referee.patch (24.5 KB) - added by mhansen 13 years ago.
trac_1707_pbuild.patch (1.3 KB) - added by gfurnish 13 years ago.
set file to C++ in pbuild
trac_1707-dancing_links.pyx-doctestfix.patch (2.2 KB) - added by mabshoff 13 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 13 years ago by mhansen

  • 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 mabshoff

  • 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 wdj

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.

comment:4 Changed 13 years ago by wdj

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

Changed 13 years ago by carlohamalainen

Dancing links C++

Changed 13 years ago by carlohamalainen

Dancing links C++ wrapper

Changed 13 years ago by carlohamalainen

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

comment:5 Changed 13 years ago by carlohamalainen

  • 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 mabshoff

  • 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: Changed 13 years ago by 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?

comment:8 in reply to: ↑ 7 Changed 13 years ago by mabshoff

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 carlohamalainen

  • 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

Changed 13 years ago by carlohamalainen

patch against 2.11.alpha1, needs review

comment:10 Changed 13 years ago by wdj

  • 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:

  1. "nauty?" should be replaced by "NICE?"
  2. add REFERENCES section, with actual literature sources
  3. allow base rings other than ZZ
  4. discuss whether isotopism sould return a Permutation or a

PermutationGroupElement? with sage-devel

comment:11 Changed 13 years ago by mhansen

  • 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 mhansen

comment:12 Changed 13 years ago by mhansen

  • 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 mabshoff

  • 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 gfurnish

set file to C++ in pbuild

Changed 13 years ago by mabshoff

comment:14 Changed 13 years ago by mabshoff

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 nthiery

  • Cc sage-combinat added
Note: See TracTickets for help on using tickets.