coefficients of symbolic expressions revamp

[rwst]

The ticket asks for

• adding a sparse parameter to Expression.coefficients(), default True
• removing/deprecating the coeff and coeffs aliases
• implement Expression.list(), simply calling coefficients(sparse=False)

This appears to be consensus for polynomials in the thread https://groups.google.com/forum/?hl=en#!topic/sage-devel/IHirUHTWnuA

Component: symbolics

Keywords: list, polynomial, coeff

Author: Ralf Stephan

Branch/Commit: 0fec129

Reviewer: John Perry

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

[rwst]

comment:1

OK, noninteger exponents will throw an exception with sparse=False. Any better idea?

[rwst]

Branch: u/rws/coefficients_of_symbolic_expressions_revamp

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Commit: 9452fa9

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

9452fa9

17438: deprecate ex.coeff/coeffs()

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

0fec129

17438: implement ex.list()

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 9452fa9 to 0fec129

[rwst]

Changed keywords from none to list, polynomial, coeff

[rwst]

Author: Ralf Stephan

[johnperry-math]

comment:7

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?

[johnperry-math]

comment:8

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

[johnperry-math]

comment:9

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 Sage-6.3 develop branch. Do I have to download Sage-6.4, or even a recent beta of Sage-6.5?

[rwst]

comment:10

The branch is based on 6.5beta1 and the newest develop is beta2. Do you want me to merge beta2 into the branch?

[johnperry-math]

comment:11

Replying to @rwst:

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.

[rwst]

comment:12

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?

[bgrenet]

comment:13

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

[johnperry-math]

comment:14

Replying to @rwst:

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?

[rwst]

comment:15

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.

[rwst]

comment:16

I forgot: Did I say that without git-trac development is tedious? Highly recommended.

[johnperry-math]

comment:17

Replying to @rwst:

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.

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 then git trac checkout 17438, and now NOT make but sage -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 git-trac 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 in-depth... 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.

[johnperry-math]

comment:18

Replying to @rwst:

... then git trac checkout 17438, and now NOT make but sage -b.

Also, I typically use sage -b, and invoke make only when first building sage. There was a discussion a while back on sage-devel 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.

[rwst]

comment:19

Replying to @johnperry-math:

... 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 git-trac but using git with the argument trac (which is set in your .git/config.

It may well be that you will only appreciate the git-trac 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 code---this does a sync with the repo.

[johnperry-math]

comment:20

Replying to @rwst:

For why git pull is done: as developer you always want to work in your own projects with the newest code---this does a sync with the repo.

Either way, I still seem stuck with a two-hour recompilation, except that at the moment I'm getting it from both the tarball and the git pull. :-)

[johnperry-math]

comment:21

After updating my develop branch per your instructions, importing your branch still prompts Sage to recompile Cython code, with near-immediate failure at sage/algebras/quatalg/quaternion_algebra_cython.pyx. (I appreciate the near-immediate 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.

[johnperry-math]

comment:22

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?

[johnperry-math]

comment:23

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.

[rwst]

comment:24

Thanks fo the review!

Replying to @johnperry-math:

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

[vbraun]

comment:25

reviewer name

[johnperry-math]

Reviewer: john_perry

[johnperry-math]

comment:26

Sorry about that.

[vbraun]

comment:27

The reviewer name is supposed to be your real name, not the trac account name.

[johnperry-math]

comment:29

"john_perry" changed to "John Perry". Let me know if it needs more changes.

[johnperry-math]

Changed reviewer from john_perry to John Perry

[vbraun]

Changed branch from u/rws/coefficients_of_symbolic_expressions_revamp to 0fec129