Opened 5 years ago

Closed 5 years ago

#17438 closed enhancement (fixed)

coefficients of symbolic expressions revamp

Reported by: rws Owned by:
Priority: major Milestone: sage-6.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) Commit: 0fec129b1055535de50cf4e5c663662e0d877742
Dependencies: Stopgaps:

Description

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

Change History (30)

comment:1 Changed 5 years ago by rws

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

comment:2 Changed 5 years ago by rws

  • Branch set to u/rws/coefficients_of_symbolic_expressions_revamp

comment:3 Changed 5 years ago by git

  • Commit set to 9452fa934b8064657743e4ba32869430304fdb13

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

9452fa917438: deprecate ex.coeff/coeffs()

comment:4 Changed 5 years ago by git

  • Commit changed from 9452fa934b8064657743e4ba32869430304fdb13 to 0fec129b1055535de50cf4e5c663662e0d877742

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

0fec12917438: implement ex.list()

comment:5 Changed 5 years ago by rws

  • Status changed from new to needs_review

comment:6 Changed 5 years ago by rws

  • Authors set to Ralf Stephan
  • Keywords list polynomial coeff added

comment:7 Changed 5 years ago by john_perry

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 5 years ago by john_perry

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 5 years ago by john_perry

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?

Last edited 5 years ago by john_perry (previous) (diff)

comment:10 follow-up: Changed 5 years ago by rws

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 5 years ago by john_perry

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.

Last edited 5 years ago by john_perry (previous) (diff)

comment:12 follow-up: Changed 5 years ago by 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?

comment:13 Changed 5 years ago by bruno

+ 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 5 years ago by john_perry

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 follow-ups: Changed 5 years ago by rws

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 5 years ago by rws

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

comment:17 in reply to: ↑ 15 ; follow-up: Changed 5 years ago by john_perry

Replying to rws:

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.

comment:18 in reply to: ↑ 15 Changed 5 years ago by john_perry

Replying to rws:

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

comment:19 in reply to: ↑ 17 ; follow-up: Changed 5 years ago by rws

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

comment:20 in reply to: ↑ 19 Changed 5 years ago by john_perry

Replying to rws:

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

comment:21 Changed 5 years ago by john_perry

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.

comment:22 follow-up: Changed 5 years ago by john_perry

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 5 years ago by john_perry

  • 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 5 years ago by rws

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:25 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

reviewer name

comment:26 Changed 5 years ago by john_perry

  • Reviewers set to john_perry
  • Status changed from needs_work to positive_review

Sorry about that.

comment:27 Changed 5 years ago by vbraun

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

comment:28 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:29 Changed 5 years ago by john_perry

  • 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 5 years ago by vbraun

  • Branch changed from u/rws/coefficients_of_symbolic_expressions_revamp to 0fec129b1055535de50cf4e5c663662e0d877742
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.