#24625 closed enhancement (fixed)

Some small improvements to polynomial_complex_arb

Reported by: mmezzarobba Owned by:
Priority: major Milestone: sage-8.2
Component: algebra Keywords:
Cc: Merged in:
Authors: Marc Mezzarobba Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: d6dd6a0 (Commits) Commit: d6dd6a0cc8bc231dacc8eb98c935db56f8fea459
Dependencies: Stopgaps:

Description


Change History (12)

comment:1 Changed 22 months ago by mmezzarobba

  • Status changed from new to needs_review

comment:2 follow-up: Changed 22 months ago by tscrim

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.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 22 months ago by mmezzarobba

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.

comment:4 Changed 22 months ago by git

  • Commit changed from 828a99ae42645a7cb472f67a87bea3d5cf233f2e to a72d138549b2fd7176d0a1d4f9bb12047f1ce4b5

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

a72d138polynomial_complex_arb: revert_series()

comment:5 in reply to: ↑ 3 ; follow-up: Changed 22 months ago by tscrim

Replying to mmezzarobba:

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.)

comment:6 in reply to: ↑ 5 Changed 22 months ago by mmezzarobba

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.

comment:7 Changed 22 months ago by git

  • Commit changed from a72d138549b2fd7176d0a1d4f9bb12047f1ce4b5 to f5b3aa3789bf8d563736b59e101311e54b523296

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

f5b3aa3polynomial_complex_arb: revert_series()

comment:8 follow-up: Changed 22 months ago by 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.

comment:9 Changed 22 months ago by git

  • Commit changed from f5b3aa3789bf8d563736b59e101311e54b523296 to d6dd6a0cc8bc231dacc8eb98c935db56f8fea459

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

d6dd6a0polynomial_complex_arb: revert_series()

comment:10 in reply to: ↑ 8 Changed 22 months ago by mmezzarobba

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:

d6dd6a0polynomial_complex_arb: revert_series()

New commits:

d6dd6a0polynomial_complex_arb: revert_series()

comment:11 Changed 22 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

No problem.

comment:12 Changed 22 months ago by vbraun

  • Branch changed from u/mmezzarobba/acb_poly to d6dd6a0cc8bc231dacc8eb98c935db56f8fea459
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.