Opened 10 years ago
Closed 8 years ago
#11840 closed defect (fixed)
sage.symbolic.expression.Expression.collect_common_factors has no documentation
Reported by:  mjo  Owned by:  mvngu 

Priority:  major  Milestone:  sage6.2 
Component:  documentation  Keywords:  symbolic, beginner 
Cc:  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Marc Mezzarobba, Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  fb4ca11 (Commits, GitHub, GitLab)  Commit:  fb4ca118515da0514bdaca99807dfa2876feab26 
Dependencies:  Stopgaps: 
Description
The collect_common_factors method has an example, but no other documentation. It is not obvious from the documentation what the method is supposed to do.
Change History (20)
comment:1 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:2 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:3 Changed 8 years ago by
 Keywords symbolic beginner added
comment:4 Changed 8 years ago by
 Branch set to u/chapoton/11840
 Commit set to 06d4fe0e2b1c40cbebd4f72ffd171ba9b2389db6
 Status changed from new to needs_review
comment:5 Changed 8 years ago by
 Reviewers set to Marc Mezzarobba
I'd put the comment about LOG_TEN_TWO_PLUS_EPSILON
before its definition rather than below. The rest looks fine to me.
comment:6 Changed 8 years ago by
 Commit changed from 06d4fe0e2b1c40cbebd4f72ffd171ba9b2389db6 to 2b111097f96020a082f669ef426e999bd68597e7
Branch pushed to git repo; I updated commit sha1. New commits:
2b11109  trac #11840 swapped two lines

comment:7 Changed 8 years ago by
Done. Many thanks for your reviews, here and in other tickets !
comment:8 Changed 8 years ago by
 Status changed from needs_review to positive_review
comment:9 Changed 8 years ago by
Conflict, please merge in latest beta
comment:10 Changed 8 years ago by
Sorry, but I do not see any conflit with 6.2.beta3. Is there any 6.2.beta4 ?
comment:11 Changed 8 years ago by
No, not yet. Conflict is then probably due to the pynac update #15198. The easiest solution will be to try again when beta4 is out.
comment:12 Changed 8 years ago by
In case it may be of use, I rebased Frédéric's branch on top of https://github.com/vbraun/sage.git/develop and uploaded the result under u/mmezzarobba/11840collect_common_factors
.
comment:13 Changed 8 years ago by
Wait, this change will also conflict with #15892, which probably should go in first.
comment:14 Changed 8 years ago by
Please merge in 6.2.beta4
comment:15 Changed 8 years ago by
 Commit changed from 2b111097f96020a082f669ef426e999bd68597e7 to f7c9326ea9da5c30b49cb7eceae2c9dd64fc9119
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
f7c9326  Merge branch 'u/chapoton/11840' of trac.sagemath.org:sage into 11840

comment:16 Changed 8 years ago by
merging done
comment:17 Changed 8 years ago by
 Branch changed from u/chapoton/11840 to public/11840
 Commit changed from f7c9326ea9da5c30b49cb7eceae2c9dd64fc9119 to fb4ca118515da0514bdaca99807dfa2876feab26
Merges fine with 6.2beta5. Docs compile and look good. I have uploaded a reviewer's patch at public/11840
. If you like it, set positive. If not, set Branch:
to your branch again and set positive.
New commits:
cedda60  Merge branch 'u/chapoton/11840' of git://trac.sagemath.org/sage into tmp

fb4ca11  trac #11840: reviewer's patch: consistent use of imperative definition

comment:18 Changed 8 years ago by
 Reviewers changed from Marc Mezzarobba to Marc Mezzarobba, Ralf Stephan
comment:19 Changed 8 years ago by
 Status changed from needs_review to positive_review
ok, then good to me too. Setting to positive review. Thanks for the review.
comment:20 Changed 8 years ago by
 Branch changed from public/11840 to fb4ca118515da0514bdaca99807dfa2876feab26
 Resolution set to fixed
 Status changed from positive_review to closed
Here is a git branch with a little bit more documentation for this method.
I have also taken the opportunity to put raise statement into python3 format, and to use the trac role to add links to the tickets.
New commits:
trac #11840 first step, plus doc python 3 and trac role cleanup
trac #11840 details, making sure that tests pass