Opened 22 months ago
Closed 22 months ago
#24625 closed enhancement (fixed)
Some small improvements to polynomial_complex_arb
Reported by:  mmezzarobba  Owned by:  

Priority:  major  Milestone:  sage8.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
 Status changed from new to needs_review
comment:2 followup: ↓ 3 Changed 22 months ago by
comment:3 in reply to: ↑ 2 ; followup: ↓ 5 Changed 22 months ago by
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 anf
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 formattedself
, which will look a little strange in the, e.g., html doc.
Thanks, I'll fix that.
comment:4 Changed 22 months ago by
 Commit changed from 828a99ae42645a7cb472f67a87bea3d5cf233f2e to a72d138549b2fd7176d0a1d4f9bb12047f1ce4b5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a72d138  polynomial_complex_arb: revert_series()

comment:5 in reply to: ↑ 3 ; followup: ↓ 6 Changed 22 months ago by
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 anf
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
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
 Commit changed from a72d138549b2fd7176d0a1d4f9bb12047f1ce4b5 to f5b3aa3789bf8d563736b59e101311e54b523296
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f5b3aa3  polynomial_complex_arb: revert_series()

comment:8 followup: ↓ 10 Changed 22 months ago by
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
 Commit changed from f5b3aa3789bf8d563736b59e101311e54b523296 to d6dd6a0cc8bc231dacc8eb98c935db56f8fea459
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d6dd6a0  polynomial_complex_arb: revert_series()

comment:10 in reply to: ↑ 8 Changed 22 months ago by
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()

comment:11 Changed 22 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
No problem.
comment:12 Changed 22 months ago by
 Branch changed from u/mmezzarobba/acb_poly to d6dd6a0cc8bc231dacc8eb98c935db56f8fea459
 Resolution set to fixed
 Status changed from positive_review to closed
A few things:
_rmul_
and_lmul_
for bad inputs and make sure they fail gracefully.revert_series
for arb, are those actual things that prevent you from finding such anf
or is it just something you cannot currently handle?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).revert_series
, you have a latex formattedself
, which will look a little strange in the, e.g., html doc.