Opened 4 years ago

Closed 3 years ago

#23423 closed defect (fixed)

divided_difference in SchubertPolynomialRing should not throw errors on unused variables

Reported by: darij Owned by:
Priority: major Milestone: sage-8.1
Component: combinatorics Keywords: schubert, polynomials, divided differences
Cc: tscrim, sage-combinat Merged in:
Authors: Darij Grinberg Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 37b4648 (Commits) Commit: 37b46486872755c05a2c096ac5423b3cc1016786
Dependencies: #23403 Stopgaps:

Description

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.

Change History (13)

comment:1 Changed 4 years ago by darij

Now solved in #23403.

comment:2 Changed 4 years ago by tscrim

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.

comment:3 Changed 4 years ago by darij

  • Branch set to public/combinat/improve_schubert_divided_difference
  • Commit set to f915925252394a15968678505299eaf256bb1d06
  • Dependencies set to #23403

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.

comment:4 Changed 4 years ago by tscrim

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.

comment:5 Changed 3 years ago by darij

  • Status changed from new to needs_review

comment:6 Changed 3 years ago by darij

Turns out no rebase is needed.

comment:7 Changed 3 years ago by tscrim

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.

comment:8 follow-up: Changed 3 years ago by darij

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.

comment:9 in reply to: ↑ 8 Changed 3 years ago by tscrim

Replying to darij:

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.

comment:10 Changed 3 years ago by git

  • Commit changed from f915925252394a15968678505299eaf256bb1d06 to 37b46486872755c05a2c096ac5423b3cc1016786

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

comment:11 Changed 3 years ago by darij

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

comment:12 Changed 3 years ago by tscrim

  • Authors set to Darij Grinberg
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thank you.

comment:13 Changed 3 years ago by vbraun

  • Branch changed from public/combinat/improve_schubert_divided_difference to 37b46486872755c05a2c096ac5423b3cc1016786
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.