Opened 2 years ago
Closed 2 years ago
#30754 closed enhancement (fixed)
refresh Cfinite sequences
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.3 |
Component: | commutative algebra | Keywords: | |
Cc: | slelievre | Merged in: | |
Authors: | Frédéric Chapoton | Reviewers: | Samuel Lelièvre |
Report Upstream: | N/A | Work issues: | |
Branch: | 0da8bca (Commits, GitHub, GitLab) | Commit: | 0da8bcad8d684c26a1a7f8a473ff2b0847a7ebc0 |
Dependencies: | Stopgaps: |
Description (last modified by )
Full flake8 and some small changes in code.
Change History (8)
comment:1 Changed 2 years ago by
Branch: | → u/chapoton/30754 |
---|---|
Commit: | → 3fdf884dcc51fd0e83f0cf4ca92a6fba867fb53d |
Status: | new → needs_review |
comment:2 Changed 2 years ago by
Commit: | 3fdf884dcc51fd0e83f0cf4ca92a6fba867fb53d → 5681d88f341d679c0704e69ce979d9370beecf59 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
5681d88 | fix pyflakes in cfinite
|
comment:3 Changed 2 years ago by
Positive review if bots are happy.
Or if you want to go further, but no pressure to do that here, some ideas.
One could check whether
- some
== 0
could be.is_zero()
- some
== 1
could be.is_one()
- some
X(0)
could beX.zero()
- some
X(1)
could beX.one()
- some
X(n / d)
could ben // d
One could consider minor rewriting in some places.
for k, a in enumerate(part.numerator()): - a = QQbar(a) + a = -QQbar(a) if k % 2 else QQbar(a) bino = binomial(n + m - k, m) - c += bino * SR(((-1)**k * a * b**(k - m - 1)).radical_expression()) + c += bino * SR((a * b**(k - m - 1)).radical_expression())
- if i != 0: + if i:
- co = coefficients[::-1] - co.extend([0] * (len(values) - deg)) - x = self.polynomial_ring().gen() - den = -1 + sum(x**(n + 1) * co[n] for n in range(deg)) - num = -values[0] + sum(x**n * (-values[n] + - sum(values[k] * co[n - 1 - k] - for k in range(n))) - for n in range(1, len(values))) + co = coefficients[::-1] + [0] * (len(values) - deg) + R = self.polynomial_ring() + den = R([-1] + co[:deg]) + v = lambda m: sum((values[k] * co[m - k] for k in range(m + 1)), -values[m + 1]) + num = R([v(n) for n in range(-1, len(values) - 1)])
- sequence = sequence[:-1] + del sequence[-1]
- l = len(sequence) - while l and sequence[l - 1] == 0: - l -= 1 - sequence = sequence[:l] + while sequence and sequence[-1] == 0: + del sequence[-1] + l = len(sequence)
- offset = next((i for i, x in enumerate(sequence) if x != 0), None) + offset = next((i for i, x in enumerate(sequence) if x), None)
Final suggestion, even more out of scope here.
Is from_recurrence
slightly off when values
is longer than coefficients
?
For example, should 6
repeat here?
sage: a = C.from_recurrence([1, 1], [0, 1, 1, 5]) sage: a[:9] [0, 1, 1, 5, 6, 6, 11, 17, 28]
comment:4 Changed 2 years ago by
Commit: | 5681d88f341d679c0704e69ce979d9370beecf59 → 0da8bcad8d684c26a1a7f8a473ff2b0847a7ebc0 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
0da8bca | more details in cfinite sequences
|
comment:5 Changed 2 years ago by
This is a bug in __getitem__
, already there before the changes made here. I have no idea how to fix that.
sage: C = CFiniteSequences(QQ) sage: a = C.from_recurrence([1, 1], [0, 1, 1, 5]) sage: a[:9] [0, 1, 1, 5, 6, 6, 11, 17, 28]
comment:6 Changed 2 years ago by
Cc: | slelievre added |
---|---|
Description: | modified (diff) |
Reviewers: | → Samuel Lelièvre |
I know the getitem bug is not from the changes here, no worries.
Thanks for the extra details you fixed following suggestions in comment:3.
I get how the lambda function suggestion was not so appealing.
Another way to streamline, hopefully more convincing:
- num = R([-values[0]] + - [-values[n] + sum(values[k] * co[n - 1 - k] - for k in range(n)) - for n in range(1, len(values))]) + num = R([-v + sum(values[k] * co[n - 1 - k] for k in range(n)) + for n, v in enumerate(values)])
since when n
is zero the sum is empty, so no need to special-case it.
But up to you and again, no pressure.
Positive review on my behalf if bots are happy, with or without this change.
comment:8 Changed 2 years ago by
Branch: | u/chapoton/30754 → 0da8bcad8d684c26a1a7f8a473ff2b0847a7ebc0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
refresh C-finite sequences (flake8 and code details)