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: |
Description (last modified by )
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)
Change History (21)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- 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.
comment:2 Changed 10 years ago by
comment:3 follow-up: ↓ 4 Changed 9 years ago by
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
comment:4 in reply to: ↑ 3 Changed 9 years ago by
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.
comment:5 Changed 9 years ago by
- Reviewers set to Peter Bruin
comment:6 follow-up: ↓ 7 Changed 9 years ago by
- 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
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.
comment:8 Changed 9 years ago by
- 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
- 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
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
applies to 5.10.rc2, but differs non-trivially from trac_13272_docstring.patch
comment:11 Changed 9 years ago by
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: ↓ 13 Changed 9 years ago by
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
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
- Milestone changed from sage-5.11 to sage-5.12
comment:15 Changed 8 years ago by
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
- 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
- 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
- Resolution set to fixed
- Status changed from positive_review to closed
remove proof keyword