Opened 7 years ago
Closed 7 years ago
#17438 closed enhancement (fixed)
coefficients of symbolic expressions revamp
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage6.5 
Component:  symbolics  Keywords:  list, polynomial, coeff 
Cc:  Merged in:  
Authors:  Ralf Stephan  Reviewers:  John Perry 
Report Upstream:  N/A  Work issues:  
Branch:  0fec129 (Commits, GitHub, GitLab)  Commit:  0fec129b1055535de50cf4e5c663662e0d877742 
Dependencies:  Stopgaps: 
Description
The ticket asks for
 adding a
sparse
parameter toExpression.coefficients()
, defaultTrue
 removing/deprecating the
coeff
andcoeffs
aliases  implement
Expression.list()
, simply callingcoefficients(sparse=False)
This appears to be consensus for polynomials in the thread https://groups.google.com/forum/?hl=en#!topic/sagedevel/IHirUHTWnuA
Change History (30)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
 Branch set to u/rws/coefficients_of_symbolic_expressions_revamp
comment:3 Changed 7 years ago by
 Commit set to 9452fa934b8064657743e4ba32869430304fdb13
Branch pushed to git repo; I updated commit sha1. New commits:
9452fa9  17438: deprecate ex.coeff/coeffs()

comment:4 Changed 7 years ago by
 Commit changed from 9452fa934b8064657743e4ba32869430304fdb13 to 0fec129b1055535de50cf4e5c663662e0d877742
Branch pushed to git repo; I updated commit sha1. New commits:
0fec129  17438: implement ex.list()

comment:5 Changed 7 years ago by
 Status changed from new to needs_review
comment:6 Changed 7 years ago by
 Keywords list polynomial coeff added
comment:7 Changed 7 years ago by
I'm looking at this now. I'm sorry for the delay; I only finished traveling this past weekend.
Unfortunately, it looks like the delay will take even longer, because after checking out your branch, Sage wants to recompile every blessed Cython file... as usual... and it just failed on 'sage/algebras/quatalg/quaternion_algebra_cython.pyx'. Great.
Any ideas?
comment:8 Changed 7 years ago by
I've left that temporary branch, deleted it, & am first rebuilding the develop branch. I'll try again when Sage quits compiling all the Cython files. The good news is that it made it past sage/algebras/quatalg/quaternion_algebra_cython.pyx
. :)
comment:9 Changed 7 years ago by
Importing fails, again. Failure occurs in the same place, but the first message that looks like an error is this:
missing cimport in module 'cpython.slice': ./sage/rings/number_field/number_field_element.pxd
Dunno how to proceed from here. FWIW I'm trying to build from my Sage6.3 develop branch. Do I have to download Sage6.4, or even a recent beta of Sage6.5?
comment:10 followup: ↓ 11 Changed 7 years ago by
The branch is based on 6.5beta1 and the newest develop is beta2. Do you want me to merge beta2 into the branch?
comment:11 in reply to: ↑ 10 Changed 7 years ago by
Replying to rws:
The branch is based on 6.5beta1 and the newest develop is beta2. Do you want me to merge beta2 into the branch?
I have no idea. Let me try downloading & building 6.5beta1 first, then applying your patch. (I imagine I can find 6.5beta1 online.) That will take a few hours, so I'll know what to ask at that point, which (for me) may be tomorrow. Sorry again for the delay.
Edit: Yup! I found 6.5beta1 online without too much work.
comment:12 followup: ↓ 14 Changed 7 years ago by
You really should use git so you don't need to download tarballs. How would you apply this branch or upload trac without git, anyway?
comment:13 Changed 7 years ago by
+ val = l[0][1]
+ if val < 0:
+ raise ValueError("Cannot return dense coefficient list with negative valuation.")
I can understand the rationale behind this ValueError
. Yet, couldn't this be useful in some cases to have a dense list of coefficients, even if the first one is not the constant coefficient? In some sense, since the user has to specify sparse=dense
, she knows what she is doing. I feel like it is better not to raise exceptions when a meaningful (and not too misleading) answer exists.
Yet, I just checked the situation for Laurent polynomials and it appears that only the list of nonzero coefficients can be computed. That is the implementation you propose is consistent with the case of Laurent polynomials.
comment:14 in reply to: ↑ 12 Changed 7 years ago by
Replying to rws:
You really should use git so you don't need to download tarballs. How would you apply this branch or upload trac without git, anyway?
I am using git, and following the sage developer manual, to boot. How would I upgrade source from 6.3 to 6.5 without downloading the latest tar ball? even if I did, how would I avoid recompiling source when there are so many changes between branches?
comment:15 followups: ↓ 17 ↓ 18 Changed 7 years ago by
Given you have cloned github.com/sagemath/sage
twice (master, develop) you checkout develop and say git pull
. Whatever version you had, now you have 6.5beta2. Whenever you git trac checkout 17438
you will have the version that I was using on upload, i.e., 6.5beta1.
The best strategy for you with an old clone would be to pull HEAD (=6.5beta2), make clean; make
then git trac checkout 17438
, and now NOT make
but sage b
. Also, installing ccache is very very recommended.
comment:16 Changed 7 years ago by
I forgot: Did I say that without gittrac
development is tedious? Highly recommended.
comment:17 in reply to: ↑ 15 ; followup: ↓ 19 Changed 7 years ago by
Replying to rws:
Given you have cloned
github.com/sagemath/sage
twice (master, develop) you checkout develop and saygit pull
. Whatever version you had, now you have 6.5beta2. Whenever yougit trac checkout 17438
you will have the version that I was using on upload, i.e., 6.5beta1.
OK, yes, I have a master; I have a develop; I had switched to develop (checkout develop
) & recompiled. But I did not do a git pull
, because the developer's manual says nothing* about that, aside from pulling, say, a particular branch (which is what I did: I pulled your branch, and compilation failed). Also, the Sage Developer's Guide says to create a new branch for a ticket (git checkout
b
my_branch FETCH_HEAD
) so that's what I did. Was that wrong, too?
The best strategy for you with an old clone would be to pull HEAD (=6.5beta2),
make clean; make
thengit trac checkout 17438
, and now NOTmake
butsage b
. Also, installing ccache is very very recommended.
I could try that, though right now I have 6.5beta1 compiling, so I might as well stick with that for the time being.
Did I say that without
gittrac
development is tedious? Highly recommended.
Lots of people recommend it, but the Developer's Guide doesn't ("you'll have to learn git eventually, anyway, so why not start now?") and I'm actually working with other projects that use git (though not very much yet, & not very indepth... just commit & push mostly).
*I now see that it says nothing about that in the section on checking out tickets. In the section on getting changes it tells the reader, git pull trac u/user/description
. Not sure if this is a contradiction, or something to be done sequentially, but I didn't infer that it was to be done sequentially, if so. So that could explain something.
comment:18 in reply to: ↑ 15 Changed 7 years ago by
Replying to rws:
... then
git trac checkout 17438
, and now NOTmake
butsage b
.
Also, I typically use sage b
, and invoke make
only when first building sage. There was a discussion a while back on sagedevel about the fact that sage b
unnecessarily forces recompilation sometimes when only a couple of Python files have changed, and that was the first I recall reading that one is supposed to make
Sage even during development at times.
comment:19 in reply to: ↑ 17 ; followup: ↓ 20 Changed 7 years ago by
Replying to john_perry:
... Also, the Sage Developer's Guide says to create a new branch for a ticket (
git checkout
b
my_branch FETCH_HEAD
) so that's what I did. Was that wrong, too?
No, there are several ways to skin a cat.
git pull trac u/user/description
. Not sure if this is a contradiction...
That is actually not gittrac
but using git with the argument trac
(which is set in your .git/config
.
It may well be that you will only appreciate the gittrac
tool when you're more familiar with git and Sage. For why git pull
is done: as developer you always want to work in your own projects with the newest codethis does a sync with the repo.
comment:20 in reply to: ↑ 19 Changed 7 years ago by
Replying to rws:
For why
git pull
is done: as developer you always want to work in your own projects with the newest codethis does a sync with the repo.
Either way, I still seem stuck with a twohour recompilation, except that at the moment I'm getting it from both the tarball and the git pull. :)
comment:21 Changed 7 years ago by
After updating my develop
branch per your instructions, importing your branch still prompts Sage to recompile Cython code, with nearimmediate failure at sage/algebras/quatalg/quaternion_algebra_cython.pyx
. (I appreciate the nearimmediate failure. Failure 30 minutes into recompilation Cython would be mildly annoying.)
I'll try anew when the fresh compile of 6.5 finishes. That could take a while; right now it's working on ppl.
comment:22 followup: ↓ 24 Changed 7 years ago by
I was able to merge it into the fresh build of 6.5. Compiled with no problems; now running doctests.
I'm going to open a new ticket "soon" for my version. Would you advise opening a new branch on top of this ticket, or opening a new branch from my develop branch & going from there?
comment:23 Changed 7 years ago by
 Status changed from needs_review to positive_review
Ran sage tp 2 a
to ensure all doctests were run, since this has potentially wide ramifications. Did not run long doctests, since I don't see how they'd reveal anything different, but I can upon request. Doctests pass, reference documentation builds, code review makes sense, new doctests included to cover deprecation &c. Positive review.
comment:24 in reply to: ↑ 22 Changed 7 years ago by
Thanks fo the review!
Replying to john_perry:
Would you advise opening a new branch on top of this ticket, or opening a new branch from my develop branch & going from there?
I rechecked that all changed code in this ticket concerns symbolic expressions. Merging would only be necessary in case of merge conflict or a dependency on that code. But the rings/
code is completely independent of the symbolics/
code, as far as I know (and it should be).
comment:26 Changed 7 years ago by
 Reviewers set to john_perry
 Status changed from needs_work to positive_review
Sorry about that.
comment:27 Changed 7 years ago by
The reviewer name is supposed to be your real name, not the trac account name.
comment:28 Changed 7 years ago by
 Status changed from positive_review to needs_work
comment:29 Changed 7 years ago by
 Reviewers changed from john_perry to John Perry
 Status changed from needs_work to positive_review
"john_perry
" changed to "John Perry". Let me know if it needs more changes.
comment:30 Changed 7 years ago by
 Branch changed from u/rws/coefficients_of_symbolic_expressions_revamp to 0fec129b1055535de50cf4e5c663662e0d877742
 Resolution set to fixed
 Status changed from positive_review to closed
OK, noninteger exponents will throw an exception with
sparse=False
. Any better idea?