clean up the factor() docstring/interface for univariate polynomials

[saraedum]

Working on #11731, I realized that the docstring for factor() could also be cleaned up.

To make reviewing easier, I moved this to a separate ticket.

Component: documentation

Author: Julian Rueth

Branch/Commit: u/pbruin/13272-factor_args_doc @ f63ffd2

Reviewer: Peter Bruin, Jeroen Demeyer

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

[saraedum]

remove proof keyword

[saraedum]

comment:1

Attachment: trac_13272_proof.patch.gz

I mostly tried to simplify the wording in some places and unify the way polynomial rings are constructed. I changed the example where _factor_univariate_polynomial() is replaced since adding factorization where none is present seemed more natural to me. I removed the test for Test that this factorization really uses ``nffactor()`` internally since it apparently did not test anything: PARI's debug output is sent to stderr so the doctests do not see it; since there is an empty line after the line F = pol.factor(), the doctests expected no output to stdout which is what actually happened.

[saraedum]

Author: Julian Rueth

[ppurka]

comment:3

Hello, some comments on your patches :)

1. You can mention class functions in your patches by writing it as :meth:`._factor_univariate_polynomial` instead of writing that as `_factor_univariate_polynomial()`. See this dev doc for more details.
2. I don't understand why you made all the changes like the one below. I think all these forms of declarations test different ways of calling and/or declaring the rings or variables. Unless I am mistaken, it should be good to leave them alone. Maybe someone who knows the code better can comment.

-            sage: C = ComplexField(100)
-            sage: R.<x> = C[]
+
+            sage: R.<x> = ComplexField(100)[]
             sage: F = factor(x^2+3); F

[saraedum]

comment:4

Thanks for having a look at my patches.

Replying to @ppurka:

1. You can mention class functions in your patches by writing it as :meth:`._factor_univariate_polynomial` instead of writing that as `_factor_univariate_polynomial()`. See this dev doc for more details.

That makes sense. I will change the docstrings accordingly.

2. I don't understand why you made all the changes like the one below. I think all these forms of declarations test different ways of calling and/or declaring the rings or variables. Unless I am mistaken, it should be good to leave them alone. Maybe someone who knows the code better can comment.

-            sage: C = ComplexField(100)
-            sage: R.<x> = C[]
+
+            sage: R.<x> = ComplexField(100)[]
             sage: F = factor(x^2+3); F

Ok. If this was indeed intended, then I can of course revert these changes. I just had the feeling that the different constructors should not be tested in the factor() method. When I was new to sage I found such docstrings quite confusing because I wondered if these different constructors had any influence on the code that was actually demonstrated.

[pjbruin]

Reviewer: Peter Bruin

[pjbruin]

comment:6

trac_13272_docstring.patch no longer applies correctly to Sage 5.10.rc1; can you rebase it?

[pjbruin]

Description changed:

--- 
+++ 
@@ -1,3 +1,6 @@
 Working on #11731, I realized that the docstring for factor() could also be cleaned up.
 
 To make reviewing easier, I moved this to a separate ticket.
+
+Apply: trac_13272_proof.patch, trac_13272_docstring.patch
+