Opened 10 years ago

Closed 8 years ago

#13272 closed task (fixed)

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

Reported by: saraedum Owned by: mvngu
Priority: trivial Milestone: sage-6.1
Component: documentation Keywords: beginner
Cc: Merged in:
Authors: Julian Rueth Reviewers: Peter Bruin, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: u/pbruin/13272-factor_args_doc (Commits, GitHub, GitLab) Commit: f63ffd27bdb7377b823e4ecdc5116accb2986ee8
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

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.

Attachments (3)

trac_13272_proof.patch (2.1 KB) - added by saraedum 10 years ago.
remove proof keyword
trac_13272_docstring.patch (19.2 KB) - added by saraedum 9 years ago.
docstring cleanup
trac_13272_docstring_new.patch (23.3 KB) - added by pbruin 9 years ago.
applies to 5.10.rc2, but differs non-trivially from trac_13272_docstring.patch

Download all attachments as: .zip

Change History (21)

Changed 10 years ago by saraedum

remove proof keyword

comment:1 Changed 10 years ago by saraedum

  • Status changed from new to needs_review

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.

Last edited 10 years ago by saraedum (previous) (diff)

comment:2 Changed 10 years ago by saraedum

  • Authors set to Julian Rueth

comment:3 follow-up: Changed 9 years ago by ppurka

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
    

comment:4 in reply to: ↑ 3 Changed 9 years ago by saraedum

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.

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

comment:5 Changed 9 years ago by pbruin

  • Reviewers set to Peter Bruin

comment:6 follow-up: Changed 9 years ago by pbruin

  • Description modified (diff)
  • Status changed from needs_review to needs_work

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

comment:7 in reply to: ↑ 6 Changed 9 years ago by saraedum

Replying to pbruin:

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.

Changed 9 years ago by saraedum

docstring cleanup

comment:8 Changed 9 years ago by saraedum

  • Status changed from needs_work to needs_review

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.

comment:9 Changed 9 years ago by pbruin

  • Status changed from needs_review to needs_work

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.

comment:10 Changed 9 years ago by saraedum

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.

Changed 9 years ago by pbruin

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

comment:11 Changed 9 years ago by pbruin

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

comment:12 follow-up: Changed 9 years ago by ppurka

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

comment:13 in reply to: ↑ 12 Changed 9 years ago by saraedum

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

comment:14 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:15 Changed 8 years ago by pbruin

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.

comment:16 Changed 8 years ago by pbruin

  • Branch set to u/pbruin/13272-factor_args_doc
  • Commit set to f63ffd27bdb7377b823e4ecdc5116accb2986ee8
  • Status changed from needs_work to needs_review

comment:17 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers changed from Peter Bruin to Peter Bruin, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:18 Changed 8 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.