Some small improvements to polynomial_complex_arb

[mezzarobba]

Component: algebra

Author: Marc Mezzarobba

Branch/Commit: d6dd6a0

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/24625

[tscrim]

comment:2

A few things:

• You should add tests for _rmul_ and _lmul_ for bad inputs and make sure they fail gracefully.
• The revert_series for arb, are those actual things that prevent you from finding such an f or is it just something you cannot currently handle?
• The generic revert_series EXAMPLES is missing a second colon :. You should also add a test the generic code (which it would be good to have a more explicit message).
• For both revert_series, you have a latex formatted self, which will look a little strange in the, e.g., html doc.

[mezzarobba]

comment:3

Thanks for your comments!

Replying to @tscrim:

• You should add tests for _rmul_ and _lmul_ for bad inputs and make sure they fail gracefully.

I thought the coercion system took care of that?

• The revert_series for arb, are those actual things that prevent you from finding such an f or is it just something you cannot currently handle?

It depends what you mean by that. For example, the reversion of a power series of valuation 2 would be a Puiseux series.

• The generic revert_series EXAMPLES is missing a second colon :. You should also add a test the generic code (which it would be good to have a more explicit message).
• For both revert_series, you have a latex formatted self, which will look a little strange in the, e.g., html doc.

Thanks, I'll fix that.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a72d138

polynomial_complex_arb: revert_series()

[sagetrac-git]

Changed commit from 828a99a to a72d138

[tscrim]

comment:5

Replying to @mezzarobba:

Thanks for your comments!

Replying to @tscrim:

• You should add tests for _rmul_ and _lmul_ for bad inputs and make sure they fail gracefully.

I thought the coercion system took care of that?

I am not completely sure. I think it makes an attempt at executing the methods if they are defined. However, it never hurts to have a few tests. :)

• The revert_series for arb, are those actual things that prevent you from finding such an f or is it just something you cannot currently handle?

It depends what you mean by that. For example, the reversion of a power series of valuation 2 would be a Puiseux series.

So there is not a theoretical limitation on the result? (This is outside my mathematical knowledge, please bear with me.)

[mezzarobba]

comment:6

Replying to @tscrim:

I am not completely sure. I think it makes an attempt at executing the methods if they are defined. However, it never hurts to have a few tests. :)

Fine :-)

So there is not a theoretical limitation on the result? (This is outside my mathematical knowledge, please bear with me.)

Sorry if my answer was not clear. It is a limitation if we want the result to be a power series (which I'd say we do, in this context), but not if we allow for more general series expansions.

[sagetrac-git]

Changed commit from a72d138 to f5b3aa3

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f5b3aa3

polynomial_complex_arb: revert_series()

[tscrim]

comment:8

Okay, thank you for the explanations and updates. If you could just add something like

raise NotImplementedError("only implemented for certain base rings")

and add a doctest showing the error is properly raised, then this is a positive review.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d6dd6a0

polynomial_complex_arb: revert_series()

[sagetrac-git]

Changed commit from f5b3aa3 to d6dd6a0

[mezzarobba]

comment:10

Replying to @tscrim:

Okay, thank you for the explanations and updates. If you could just add something like

raise NotImplementedError("only implemented for certain base rings")

and add a doctest showing the error is properly raised, then this is a positive review.

Done, thanks a lot for the quick review!

---

New commits:

d6dd6a0

polynomial_complex_arb: revert_series()

---

New commits:

d6dd6a0

polynomial_complex_arb: revert_series()

[tscrim]

comment:11

No problem.

[tscrim]

Reviewer: Travis Scrimshaw

[vbraun]

Changed branch from u/mmezzarobba/acb_poly to d6dd6a0