Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

divided_difference in SchubertPolynomialRing should not throw errors on unused variables #23423

Closed
darijgr opened this issue Jul 13, 2017 · 18 comments

Comments

@darijgr
Copy link
Contributor

darijgr commented Jul 13, 2017

sage: X = SchubertPolynomialRing(ZZ)
sage: a = X([1,2,3,4,5])
sage: a.divided_difference(4)

This should yield 0, not an error. Elements of X are polynomials in infinitely many variables; when they don't use a variable, it doesn't mean that this variable cannot be divided-differenced over.

Followup to #23403.

Depends on #23403

CC: @tscrim @sagetrac-sage-combinat

Component: combinatorics

Keywords: schubert, polynomials, divided differences

Author: Darij Grinberg

Branch/Commit: 37b4648

Reviewer: Travis Scrimshaw

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

@darijgr darijgr added this to the sage-8.1 milestone Jul 13, 2017
@darijgr
Copy link
Contributor Author

darijgr commented Jul 16, 2017

comment:1

Now solved in #23403.

@tscrim
Copy link
Collaborator

tscrim commented Jul 16, 2017

comment:2

It is an orthogonal issue from #23403, and this would be a great place to discuss this as there could be a good use case for considering having a finite number of variables and having this throw an error. I agree with the change, but it is better documented by having a separate ticket for it.

@darijgr
Copy link
Contributor Author

darijgr commented Jul 16, 2017

comment:3

As discussed with Travis, this ticket is now the place for branch public/combinat/improve_schubert_divided_difference , which builds on top of #23403. Will need rebasing, though, once #23403 is p_r'd.

@darijgr
Copy link
Contributor Author

darijgr commented Jul 16, 2017

Commit: f915925

@darijgr
Copy link
Contributor Author

darijgr commented Jul 16, 2017

Dependencies: #23403

@darijgr
Copy link
Contributor Author

darijgr commented Jul 16, 2017

@tscrim
Copy link
Collaborator

tscrim commented Jul 16, 2017

comment:4

Okay, I did some more investigating. The biggest time sink in the symmetrica version is not symmetrica, but instead most of the time is spent on checking the input via reduced_word. Even with removing that check, enough time is spent on the overhead that even the non-optimized native algorithm beats going to/from symmetrica unless maybe I really spent some time optimizing the interface, which, IMO, is time not well spent.

@darijgr
Copy link
Contributor Author

darijgr commented Sep 12, 2017

comment:6

Turns out no rebase is needed.

@tscrim
Copy link
Collaborator

tscrim commented Sep 15, 2017

comment:7

We should change newm to algorithm, which takes 'sage' or 'symmetrica'. I also think a better check is if i in ZZ: too, but that isn't something I will quibble over. Once that is done (with corresponding documentation updates), this will be a positive review.

@darijgr
Copy link
Contributor Author

darijgr commented Sep 16, 2017

comment:8

Is that something I should do, or you want to do?

I'm fine with if i in ZZ instead of if isinstance(i, (Integer, int)), if this is what you mean.

@tscrim
Copy link
Collaborator

tscrim commented Sep 16, 2017

comment:9

Replying to @darijgr:

Is that something I should do, or you want to do?

I can do it, but it might take me a little longer to push right now. However, it was somewhat a question of seeing if you agree; granted, I didn't phrase it like I should have.

I'm fine with if i in ZZ instead of if isinstance(i, (Integer, int)), if this is what you mean.

Yes, it is. The rationale: if by chance a user is doing 2/2, it will still work. Granted, this is not likely to happen, but I've had it happen to me enough that I'm paranoid about it now.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 16, 2017

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

a5b2cffMerge branch 'public/combinat/improve_schubert_divided_difference' of git://trac.sagemath.org/sage into schub
37b4648changes suggested by Travis

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 16, 2017

Changed commit from f915925 to 37b4648

@darijgr
Copy link
Contributor Author

darijgr commented Sep 16, 2017

comment:11

Done those changes. But be warned that I haven't tested the correct behavior of the 'symmetrica' algorithm on polynomials with too few variables.

@tscrim
Copy link
Collaborator

tscrim commented Sep 17, 2017

Author: Darij Grinberg

@tscrim
Copy link
Collaborator

tscrim commented Sep 17, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Sep 17, 2017

comment:12

Thank you.

@vbraun
Copy link
Member

vbraun commented Sep 20, 2017

Changed branch from public/combinat/improve_schubert_divided_difference to 37b4648

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants