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:

Status badges

Description (last modified by slelievre)

Full flake8 and some small changes in code.

Change History (8)

comment:1 Changed 2 years ago by chapoton

Branch: u/chapoton/30754
Commit: 3fdf884dcc51fd0e83f0cf4ca92a6fba867fb53d
Status: newneeds_review

New commits:

3fdf884refresh C-finite sequences (flake8 and code details)

comment:2 Changed 2 years ago by git

Commit: 3fdf884dcc51fd0e83f0cf4ca92a6fba867fb53d5681d88f341d679c0704e69ce979d9370beecf59

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

5681d88fix pyflakes in cfinite

comment:3 Changed 2 years ago by slelievre

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 be X.zero()
  • some X(1) could be X.one()
  • some X(n / d) could be n // 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 git

Commit: 5681d88f341d679c0704e69ce979d9370beecf590da8bcad8d684c26a1a7f8a473ff2b0847a7ebc0

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

0da8bcamore details in cfinite sequences

comment:5 Changed 2 years ago by chapoton

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 slelievre

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:7 Changed 2 years ago by chapoton

Status: needs_reviewpositive_review

so positive review, thanks

comment:8 Changed 2 years ago by vbraun

Branch: u/chapoton/307540da8bcad8d684c26a1a7f8a473ff2b0847a7ebc0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.