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
comment:2 Changed 4 years ago by
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
- Branch set to public/combinat/improve_schubert_divided_difference
- Commit set to f915925252394a15968678505299eaf256bb1d06
- Dependencies set to #23403
comment:4 Changed 4 years ago by
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
- Status changed from new to needs_review
comment:6 Changed 3 years ago by
Turns out no rebase is needed.
comment:7 Changed 3 years ago by
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: ↓ 9 Changed 3 years ago by
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
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 ofif 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
- Commit changed from f915925252394a15968678505299eaf256bb1d06 to 37b46486872755c05a2c096ac5423b3cc1016786
comment:11 Changed 3 years ago by
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
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Thank you.
comment:13 Changed 3 years ago by
- Branch changed from public/combinat/improve_schubert_divided_difference to 37b46486872755c05a2c096ac5423b3cc1016786
- Resolution set to fixed
- Status changed from positive_review to closed
Now solved in #23403.