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

saraedum · 2012-07-19T13:48:14Z

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

The text was updated successfully, but these errors were encountered:

saraedum · 2012-07-19T19:50:13Z

remove proof keyword

saraedum · 2012-07-19T20:01:26Z

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 · 2012-07-19T22:10:52Z

Author: Julian Rueth

ppurka · 2012-07-26T06:19:02Z

comment:3

Hello, some comments on your patches :)

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.
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 · 2012-07-26T12:10:52Z

comment:4

Thanks for having a look at my patches.

Replying to @ppurka:

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.

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 · 2013-06-15T16:21:04Z

Reviewer: Peter Bruin

pjbruin · 2013-06-15T16:29:02Z

comment:6

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

saraedum · 2013-06-16T21:53:53Z

comment:7

Replying to @pjbruin:

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

Thanks for looking at these tickets. I'm building 5.10 now, will rebase the patch when this is done.

saraedum · 2013-06-18T14:15:47Z

docstring cleanup

saraedum · 2013-06-18T14:20:32Z

comment:8

Attachment: trac_13272_docstring.patch.gz

I rebased the patch. It might not apply anymore, since I rebased it against the git repository and used sagedev's export_patch to export it. I'm not sure if the patchbot cares about whitespace since that will most probably be incorrect.

pjbruin · 2013-06-18T14:44:59Z

comment:9

Unfortunately the whitespace matters a lot:

sage: hg_sage.qapplied()
cd "/home/staff/pbruin/src/sage-5.10.rc2/devel/sage" && sage --hg qapplied  
trac_13272_proof.patch
sage: hg_sage.qpush()
cd "/home/staff/pbruin/src/sage-5.10.rc2/devel/sage" && sage --hg qpush  
applying trac_13272_docstring.patch
patching file sage/rings/polynomial/polynomial_element.pyx
Hunk #1 FAILED at 2464
Hunk #2 FAILED at 2504
Hunk #3 FAILED at 2518
Hunk #5 FAILED at 2577
Hunk #6 FAILED at 2598
Hunk #8 FAILED at 2691
Hunk #9 FAILED at 2719
Hunk #10 FAILED at 2741
Hunk #12 FAILED at 2809
Hunk #13 FAILED at 2933
Hunk #14 FAILED at 3020
11 out of 14 hunks FAILED -- saving rejects to file sage/rings/polynomial/polynomial_element.pyx.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_13272_docstring.patch

In the meantime I did manage to rebase the first version of your patch, but even apart from whitespace the resulting patch has several differences with your new patch.

saraedum · 2013-06-18T15:09:03Z

comment:10

Could you post the rebased patch?

Sorry for bothering you with this but I don't have mercurial sage anymore. I started a thread on sage-git to find out if there is any way to make hg ignore whitespace.

pjbruin · 2013-06-18T15:13:17Z

Attachment: trac_13272_docstring_new.patch.gz

applies to 5.10.rc2, but differs non-trivially from trac_13272_docstring.patch

pjbruin · 2013-06-18T15:15:01Z

comment:11

I tried to find out if hg_sage.qpush() has an option similar to patch --ignore--whitespace, but no luck so far.

ppurka · 2013-06-18T15:31:02Z

comment:12

Please don't ignore whitespace and fuzz. It can lead to patches being inserted in the wrong place in the file.

saraedum · 2013-06-18T16:02:26Z

comment:13

Replying to @ppurka:

Please don't ignore whitespace and fuzz. It can lead to patches being inserted in the wrong place in the file.

I see your point. However, I think it could help a lot with the testing of the sage-git tools, if one could move patches between these two worlds somehow. It's probably better to move this discussion to https://groups.google.com/forum/?fromgroups#!topic/sage-git/eCPmLRqHR0s

pjbruin · 2013-12-19T21:24:43Z

comment:15

After the switch to Git, I thought it was a good time to come back to this ticket (and the others related to #11731). I converted the two patches into a branch based on Sage 6.0 (to be pushed shortly). I omitted most of the changes in the polynomial constructors (see comment:3) and did not delete the nffactor() test (see comment:1; it does see the stderr output). Furthermore, I made a few small cosmetic fixes. If you are happy with this version, feel free to set it to positive review.

pjbruin · 2013-12-19T21:27:29Z

Branch: u/pbruin/13272-factor_args_doc

pjbruin · 2013-12-19T21:27:29Z

Commit: f63ffd2

jdemeyer · 2014-01-09T17:33:39Z

Changed reviewer from Peter Bruin to Peter Bruin, Jeroen Demeyer

saraedum added this to the sage-5.11 milestone Jul 19, 2012

saraedum added c: documentation labels Jul 19, 2012

saraedum assigned sagetrac-mvngu Jul 19, 2012

saraedum added the s: needs review label Jul 19, 2012

This comment has been minimized.

Sign in to view

pjbruin added s: needs work and removed s: needs review labels Jun 15, 2013

saraedum added s: needs review and removed s: needs work labels Jun 18, 2013

pjbruin added s: needs work and removed s: needs review labels Jun 18, 2013

jdemeyer removed this from the sage-5.11 milestone Aug 13, 2013

jdemeyer added this to the sage-5.12 milestone Aug 13, 2013

pjbruin added s: needs review and removed s: needs work labels Dec 19, 2013

This comment has been minimized.

Sign in to view

jdemeyer added s: positive review and removed s: needs review labels Jan 9, 2014

vbraun removed the s: positive review label Jan 16, 2014

vbraun closed this as completed in d73e53a Jan 16, 2014

haikona mannequin mentioned this issue Dec 29, 2022

Transfer ring-specific functionality of factor() and roots() in polynomial_element.pyx to the correct ring files #11731

Open

saraedum mentioned this issue Feb 26, 2014

Add QQ._factor_univariate_polynomial() #13274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

saraedum commented Jul 19, 2012

saraedum commented Jul 19, 2012

saraedum commented Jul 19, 2012

saraedum commented Jul 19, 2012

ppurka commented Jul 26, 2012

saraedum commented Jul 26, 2012

pjbruin commented Jun 15, 2013

pjbruin commented Jun 15, 2013

This comment has been minimized.

saraedum commented Jun 16, 2013

saraedum commented Jun 18, 2013

saraedum commented Jun 18, 2013

pjbruin commented Jun 18, 2013

saraedum commented Jun 18, 2013

pjbruin commented Jun 18, 2013

pjbruin commented Jun 18, 2013

ppurka commented Jun 18, 2013

saraedum commented Jun 18, 2013

pjbruin commented Dec 19, 2013

pjbruin commented Dec 19, 2013

pjbruin commented Dec 19, 2013

This comment has been minimized.

jdemeyer commented Jan 9, 2014

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

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

Comments

saraedum commented Jul 19, 2012

saraedum commented Jul 19, 2012

saraedum commented Jul 19, 2012

saraedum commented Jul 19, 2012

ppurka commented Jul 26, 2012

saraedum commented Jul 26, 2012

pjbruin commented Jun 15, 2013

pjbruin commented Jun 15, 2013

This comment has been minimized.

saraedum commented Jun 16, 2013

saraedum commented Jun 18, 2013

saraedum commented Jun 18, 2013

pjbruin commented Jun 18, 2013

saraedum commented Jun 18, 2013

pjbruin commented Jun 18, 2013

pjbruin commented Jun 18, 2013

ppurka commented Jun 18, 2013

saraedum commented Jun 18, 2013

pjbruin commented Dec 19, 2013

pjbruin commented Dec 19, 2013

pjbruin commented Dec 19, 2013

This comment has been minimized.

jdemeyer commented Jan 9, 2014