#3514 closed enhancement (fixed)
[with patch, positive review] Free modules revision
Reported by: | kohel | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-3.1 |
Component: | algebra | Keywords: | free modules editor_mhansen |
Cc: | craigcitro, ncalexan | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
This separates quadratic modules into free_quadratic_module.py -- these are free modules with a user-specified inner product.
This adds 100% documentation to free_module.py and free_quadratic_module.py.
TODO: Probably we want to revise free module elements to make efficient use of diagonal inner_product_matrices. I still intend to generalize the inner product matrix to support different image ring (real, complex, p-adic) for the pairing, as well as integral pairings which are given by rational matrices.
Attachments (4)
Change History (23)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Summary changed from Free modules revision to [with patch, needs review] Free modules revision
comment:2 Changed 11 years ago by
- Summary changed from [with patch, needs review] Free modules revision to [with patch, with partial review but needs work] Free modules revision
Review by John Cremona:
- I read through the code fairly carefully, and was very impressed by the thought which has gone into this. There are a few typos in docstrings but nothing serious.
- I tried applying the patch to 3.0.4.alpha0 but it failed:
applying /home/jec/free-modules.patch patching file sage/modules/free_module.py Hunk #4 FAILED at 280 Hunk #52 FAILED at 3200 Hunk #54 FAILED at 3310 Hunk #82 FAILED at 4430 Hunk #95 FAILED at 5053 5 out of 96 hunks FAILED -- saving rejects to file sage/modules/free_module.py.rej abort: patch failed to apply
I expect that this is because it is based on a different release. If I get time I'll try it on 3.0.3, but it would be much better if the author could re-base it!
Should we not have a requirement that the base version for posted patches whould always be specified (like I do...)?
comment:3 Changed 11 years ago by
- Cc craigcitro added
comment:4 Changed 11 years ago by
- Cc nalexander added
This patch applies to 3.02 (sorry for not specifying, John).
Nick Alexander expressed an interest in review this patch, and looked over some of it already at SAGE-Devel days.
This predates changes merging the coercion branch, so should first be patched to a pre-coercion SAGE.
comment:5 Changed 11 years ago by
I happened to have a 3.0.2 build lying around, but it fared no better:
john@ubuntu%./sage ---------------------------------------------------------------------- | SAGE Version 3.0.2, Release Date: 2008-05-24 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- Loading SAGE library. Current Mercurial branch is: 3514 sage: hg_sage.apply ("fre free_module_element frequency_distribution sage: hg_sage.apply("/home/john/free-modules.patch") cd "/home/john/sage-3.0.2/devel/sage" && hg status cd "/home/john/sage-3.0.2/devel/sage" && hg status cd "/home/john/sage-3.0.2/devel/sage" && hg import "/home/john/free-modules.patch" applying /home/john/free-modules.patch patching file sage/modules/free_module.py Hunk #4 FAILED at 280 Hunk #54 FAILED at 3312 Hunk #82 FAILED at 4432 Hunk #95 FAILED at 5055 4 out of 96 hunks FAILED -- saving rejects to file sage/modules/free_module.py.rej abort: patch failed to apply
comment:6 Changed 11 years ago by
- Summary changed from [with patch, with partial review but needs work] Free modules revision to [with patch, needs review] Free modules revision
Rebased against 3.0.4.alpha1 and fixed all trivial errors/doctest problems. This now needs review.
comment:7 Changed 11 years ago by
The patch posted by Gary does not apply cleanly against 3.0.4.alpha1 or 2:
sage-3.0.4.alpha2/devel/sage$ patch -p1 --dry-run < trac_3514.patch\?format\=raw patching file sage/modules/all.py patching file sage/modules/free_module.py patching file sage/modules/free_quadratic_module.py patching file sage/modules/quotient_module.py patching file sage/coding/code_constructions.py patching file sage/modular/modform/hecke_operator_on_qexp.py patching file sage/modular/modsym/space.py patching file sage/modules/free_module.py Hunk #1 succeeded at 264 (offset -17 lines). Hunk #2 succeeded at 289 (offset -17 lines). Hunk #3 FAILED at 444. Hunk #4 FAILED at 872. Hunk #5 FAILED at 3044. Hunk #6 succeeded at 2843 (offset -390 lines). Hunk #7 succeeded at 2945 (offset -388 lines). Hunk #8 FAILED at 3436. Hunk #9 succeeded at 4140 (offset -927 lines). Hunk #10 succeeded at 4204 (offset -927 lines). 4 out of 10 hunks FAILED -- saving rejects to file sage/modules/free_module.py.rej patching file sage/modules/quotient_module.py patching file sage/rings/number_field/order.py patching file sage/schemes/elliptic_curves/period_lattice.py
David also posted a diff and Gary's patch commits all of the changes in Gary's name, which is obviously not correct.
Cheers,
Michael
Changed 11 years ago by
comment:9 Changed 11 years ago by
I've attached trac_3514.2.patch which applies cleanly against 3.0.4.rc0.
comment:10 Changed 11 years ago by
- Summary changed from [with patch, needs review] Free modules revision to [with patch, positive review pending minor changes] Free modules revision
Overall, I think that the patch is really good since it adds lots of good documentation and cleans things up by separating out the QuadraticSpaces?. There are a few minor things that I'd like to see addressed:
1) There are a number of "naked excepts" on lines 607,2118,2123,2239,2244,... of free_module.py that really should specify the particular type of error they are trying to catch.
2) The tests for a a couple functions that raise NotImplementedErrors? do not actually test the function. The ones I saw were hash on 458, cmp on 770, and echelonized_basis_matrix on 982 of free_module.py.
3) It seems there is a lot of code duplication in the span_of_basis's that could be factored out.
4) The BUG listed on line 4564 actually works fine for me. On a related note, you need to write special code (using the reduce method) if you want loads(dumps(s)) to be the exact same object as s. This is relevant to line 111.
comment:11 Changed 11 years ago by
- Summary changed from [with patch, positive review pending minor changes] Free modules revision to [with patch, needs review] Free modules revision
I added trac_3514-review.patch that addresses the issues above. It'd be good for someone to review my new patch -- it should be pretty easy.
I deviated a bit from my own review comments in a few places.
2) I removed hash and cmp since they didn't serve any purpose, but I left echelonized_basis_matrix because the docstring provided nontrivial information for any subclass that hasn't implemented echelonized_basis_matrix.
3) After looking at the code, I thought it was more clear to leave the little bit of duplication that was there.
4) I removed line 111 since loads(dumps()) isn't guaranteed to return the exact same object.
Changed 11 years ago by
comment:12 Changed 11 years ago by
Switching the order of the arguments in span
is backwards incompatible (and I have seen code that will break under this switch). The old ordering should still be accepted even if you want to make it so the basering is optional.
In that same function, testing {{{ if not isinstance(R, principal_ideal_domain.PrincipalIdealDomain?) }}}
is potentially brittle, as there are rings which are PID's depending on their parameters (e.g. polynomial rings, Z/nZ, etc.
comment:13 Changed 11 years ago by
- Cc ncalexan added; nalexander removed
comment:14 Changed 11 years ago by
I read the review patch by Mike Hansen. I haven't doctested it. I definitely agree with the changes he made though.
comment:15 Changed 11 years ago by
- Milestone changed from sage-3.1.1 to sage-3.1
The two patches applied after #3652 pass doctests in my current 3.1.alpha1 merge tree. So once we get a positive review here we can finally merge.
Cheers,
Michael
comment:16 Changed 11 years ago by
- Summary changed from [with patch, needs review] Free modules revision to [with patch, positive review] Free modules revision
I think this looks good. I looked over the original patches, and also mike's comments, and this looks good. I'm very impressed by the amount of new doctests, etc. Yeah!
comment:17 Changed 11 years ago by
- Resolution set to fixed
- Status changed from new to closed
Merged trac_3514.2.patch and trac_3514-review.patch in Sage 3.1.alpha1
comment:18 Changed 11 years ago by
Robert's comment: "Switching the order of the arguments in span is backwards incompatible (and I have seen code that will break under this switch). The old ordering should still be accepted even if you want to make it so the basering is optional."
I agree! See patch.
Changed 11 years ago by
comment:19 Changed 11 years ago by
Merged sage-3514_span.patch in Sage 3.1.alpha1
free_modules.patch