Opened 10 years ago
Closed 9 years ago
#11556 closed enhancement (fixed)
Linear transformations, built from free module morphisms
Reported by: | rbeezer | Owned by: | jason, was |
---|---|---|---|
Priority: | major | Milestone: | sage-4.8 |
Component: | linear algebra | Keywords: | |
Cc: | jason | Merged in: | sage-4.8.alpha4 |
Authors: | Rob Beezer | Reviewers: | Martin Raum, Jason Grout |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #11552, #11553 | Stopgaps: |
Description (last modified by )
This patch builds vector space morphisms, aka linear transformations, from free module morphisms. This allows for a few specialized methods, such as an easier test for invertibility (check the rank of a matrix representation). But it is mostly about (a) a "linear transformation" constructor for beginners' use, (b) lots of documentation, (c) specialized output routines, so it is clear when a morphism runs between two vector spaces (not just two free modules).
(c) required lots of doctest changes. When the example was complicated and involved two vector spaces, I usually changed the output to match the new format for the new morphisms. When the example was simple, I tried to "roll it back" to involve two free modules, to fully exercise that code.
Additionally, there were a lot of doctests with matrices of the wrong size, reversing domain and codomain, that managed to pass due to the bug listed in #10793. Tighter controls here required fixing a lot of these.
Depends:
Apply:
to the Sage library.
Attachments (10)
Change History (39)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Cc jason added
Patch is mostly up for safe-keeping.
(a) Passes most tests in sage/modules
, failures are just output formatting.
(b) Functional, see module-level docs for vector_space_homspace.py
.
(c) vector_space_morphisms.py
remains to be overhauled - significant module-level documentation, a friendly constructor, and general touch-up.
(d) Need to add two new files to the overall reference documentation.
comment:2 Changed 10 years ago by
- Dependencies set to #11552, #11553
- Description modified (diff)
- Status changed from new to needs_review
Changed 10 years ago by
comment:3 Changed 10 years ago by
Changed 10 years ago by
comment:4 Changed 10 years ago by
- Description modified (diff)
v4 patch adds making full-spelling aliases for minpoly and charpoly of free module morphisms, in line with the nomenclature for matrices.
comment:5 follow-up: ↓ 6 Changed 10 years ago by
- Reviewers set to Martin Raum
Hi Robert,
There are very few changes that I propose for this patch. Sorry, for not making them myself, but I somehow happened to lack time again.
- Add newline at the end of free_module_morphism.py.
- In vector_space_morphism.py l.475f it is not clear what side refers to. Delete the sentence?
- l. 639 of the same file contains a typo.
- l. 689ff do the imports globally. If for conflicts you cannot move the import statements to the file's head, move at least as many as you can.
- l. 756ff is indeed no sufficient to check for linearity. Is there a specific reason why you don't use is_polynomial()? Then check the degree and you are fine.
- l. 775 do you want to keep that line or rather merge with l. 732?
- l. 856 move this import if possible
With these very minor changes made, this ticket deserves a positive review.
comment:6 in reply to: ↑ 5 Changed 10 years ago by
Replying to mraum:
- In vector_space_morphism.py l.475f it is not clear what side refers to.
Delete the sentence?
Actual output from _repr_()
once made it clearer, but I trimmed the amount of output later. I will expand the sentence.
- l. 756ff is indeed no sufficient to check for linearity. Is there a
specific reason why you don't use is_polynomial()? Then check the degree and you are fine.
I did not think I could fully protect against all possible symbolic expressions sneaking through. But I had not thought about is_polynomial() which sounds like a good idea.
- l. 775 do you want to keep that line or rather merge with l. 732?
Mistaken leftover from various attempts to get unique parents. ;-)
- l. 856 move this import if possible
Generally: I did some reading a few months ago about imports, and got the impression that there was very little cost to placing within a method a statement like
import foo.bar
and then subsequently calling as
answer = foo.bar.stuff()
This is not exactly what is happening at l. 856, but on other tickets, you have suggested moving imports. What is the best strategy? I thought in module headers it would count against startuptime? Guess I'm just confused on what is best.
Thanks for all the help on all these tickets. It is really good to see them all getting wrapped up.
Rob
comment:7 Changed 10 years ago by
Concerning the imports and the performance, the following link might help: http://stackoverflow.com/questions/477096/python-import-coding-style
It exposes my suggestion as premature, and if you like to keep the imports as they are, do so.
comment:8 Changed 10 years ago by
Hi Martin,
Caught me just in time on the imports. Thanks for the link, that was helpful and confirmed my understanding from before. I can do a bit of cleanup on the imports, and will move one that is used twice to the top of the file. Otherwise, I will largely leave all those imports in the constructor since I think it makes the rest of the module easier to read with all those segregated.
I'm not sure is_polynomial()
is going to work. Consider:
sage: var('x y') (x, y) sage: g(x, y) = x*y sage: g.is_polynomial(x) True sage: g.is_polynomial(y) True sage: g.degree(x) 1 sage: g.degree(y) 1
I think I can do worse, with negative and/or fractional exponents. I'm open to ideas, but I guess I'm back to feeling there is no good way to confirm an arbitrary (callable) symbolic expression as being multivariate linear (including no constant term). Will post a patch of changes in a few minutes, or later today.
Rob
Changed 10 years ago by
comment:9 Changed 10 years ago by
- Description modified (diff)
"edits" patch addresses all of the reviewer suggestions, except linearity testing. I'm open to trying new things to improve error-checking for linear function input.
comment:10 follow-up: ↓ 11 Changed 10 years ago by
There is one thing left. Sorry, for this, but I probably wasn't very precise. In 2. you added a bit more explanation, but I was referring to the fact that the keyword side only influences matrices. Many lines above there is one example that demonstrates how it works. The sentence that I referred to is placed before a series of examples dealing with lambda expressions and symbolic functions. Do I get this wrong?
Concerning the linear polynomials you are probably right. I thought about something like extracting linear coefficients and then trying to recombine them. But that would have a great impact on performance. Also, lambda expressions cannot be tested for linearity either.
Changed 10 years ago by
comment:11 in reply to: ↑ 10 Changed 10 years ago by
Replying to mraum:
Do I get this wrong?
No, my mistake. Sorry about that. I thought the examples following used the side
keyword, but obviously I did not look very carefully.
"edits-v2" patch just deletes the sentence, but has some discussion elsewhere about the side. So I think this is ready for review now.
Changed 10 years ago by
comment:12 follow-up: ↓ 13 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
Great! I only changed one type (one -> on), thus the new patch.
comment:13 in reply to: ↑ 12 Changed 10 years ago by
- Description modified (diff)
Replying to mraum:
Great! I only changed one type (one -> on), thus the new patch.
And the underscore in the filename to a hyphen... ;-)
comment:14 Changed 10 years ago by
- Dependencies changed from #11552, #11553 to #11552 #11553
comment:15 follow-up: ↓ 16 Changed 10 years ago by
- Status changed from positive_review to needs_work
- Work issues set to Rebase on Sage 4.7.2.alpha2.
Or maybe the patches are out of sync with their dependencies.
comment:16 in reply to: ↑ 15 Changed 10 years ago by
Replying to leif:
Or maybe the patches are out of sync with their dependencies.
Thanks, Leif. I'm taking a short break from Sage work, but will attend to the easy ones (like this rebase) very soon. I've got a couple outstanding items on tickets you've touched, but feel free to ping me on anything critical. (I need to start by building the latest alpha!)
Thanks for your work as release manager.
Changed 10 years ago by
comment:17 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues Rebase on Sage 4.7.2.alpha2. deleted
A variety of doctests were fixed on #10793 and I fixed them too here, since code here was more strict than before and picked up the same error. So hunk failures were "correct" since they were not needed with #10793 merged. v5 version of main patch mostly just re-exports the v4 version without the failures. I had to fix one hunk by hand where my changes were a mix of #10793 changes and truly-new changes. Passes tests on affected files and sage/modules.
I will mark this "needs review" though I am not sure that is strictly needed. Feel free to approve my rebase as not needing review. (Rebase is against 4.7.2.alpha2.) I'll rebase #11553 which is just a single fuzz warning.
comment:18 Changed 10 years ago by
Trying to help the build bot
Apply trac_11556-linear-transformations-v5.patch trac-11556-linear-transformations-edits-v3.patch
comment:19 follow-up: ↓ 20 Changed 9 years ago by
What should be reviewed? Just the rebase?
comment:20 in reply to: ↑ 19 Changed 9 years ago by
Replying to jason:
What should be reviewed? Just the rebase?
I think that is right. v5 patch is a rebase on Martin's positive review at comment 12. There was some duplication of effort prior to the rebase, I was fixing some incorrect doctests, while another patch was doing the same thing. So IIRC those are the only real changes in the rebase (ie it was not a trivial matter, but if this applies and passes tests now then it should be ready to go).
Thanks, Rob
comment:21 Changed 9 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
all long doctests pass (with the very minor v5-rebase patch), so back to positive review!
Changed 9 years ago by
comment:22 Changed 9 years ago by
- Reviewers changed from Martin Raum to Martin Raum, Jason Grout
Thanks, Jason - I really need this one for classes this spring.
comment:23 Changed 9 years ago by
rbeezer: me too.
comment:24 Changed 9 years ago by
- Dependencies changed from #11552 #11553 to #11552, #11553
- Status changed from positive_review to needs_work
- Work issues set to rebase
"there will be a reject that is taken care of in the next patch" is not a proper way to post patches. Rebasing a patch means editing the original patch and not adding a patch on top of a failed(!) patch. Or, you could simply post the failing patch without the rejected hunk.
comment:25 follow-up: ↓ 26 Changed 9 years ago by
Sorry; I was debating whether to combine them or to post the very small fix separately so it could be reviewed separately. I'll combine them and post a patch.
comment:26 in reply to: ↑ 25 Changed 9 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues rebase deleted
Replying to jason:
I'll combine them and post a patch.
Beat you to it. Combined the three patches into one. I hope reviewers (Grout, Raum) don't mind that now it only has my name on it, but I wanted to finish this one off.
Passes all long tests on 4.8.alpha3, so the reject should have been handled correctly, since it involved changes to a doctest.
I've marked this "needs review", though the work done was totally trivial.
comment:27 Changed 9 years ago by
I won't have time in this last week of classes/finals to look at this. I'd say, if it passes long tests and the work was totally trivial, mark it positive review.
comment:28 Changed 9 years ago by
- Status changed from needs_review to positive_review
comment:29 Changed 9 years ago by
- Merged in set to sage-4.8.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
Safe-keeping patch