Opened 2 years ago

Closed 2 years ago

# refresh Cfinite sequences

Reported by: Owned by: chapoton major sage-9.3 commutative algebra slelievre Frédéric Chapoton Samuel Lelièvre N/A 0da8bca 0da8bcad8d684c26a1a7f8a473ff2b0847a7ebc0

Full flake8 and some small changes in code.

### comment:1 Changed 2 years ago by chapoton

Branch: → u/chapoton/30754 → 3fdf884dcc51fd0e83f0cf4ca92a6fba867fb53d new → needs_review

New commits:

 ​3fdf884 `refresh C-finite sequences (flake8 and code details)`

### comment:2 Changed 2 years ago by git

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 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( * (len(values) - deg))
-        x = self.polynomial_ring().gen()
-        den = -1 + sum(x**(n + 1) * co[n] for n in range(deg))
-        num = -values + 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] +  * (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

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

 ​0da8bca `more 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 modified (diff) → 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] +
-                [-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_review → positive_review

so positive review, thanks

### comment:8 Changed 2 years ago by vbraun

Branch: u/chapoton/30754 → 0da8bcad8d684c26a1a7f8a473ff2b0847a7ebc0 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.