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: sage-6.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:

Status badges

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 jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:2 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 8 years ago by dkrenn

  • Keywords symbolic beginner added

comment:4 Changed 8 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/11840
  • Commit set to 06d4fe0e2b1c40cbebd4f72ffd171ba9b2389db6
  • Status changed from new to needs_review

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:

​eeeb293trac #11840 first step, plus doc python 3 and trac role cleanup
​06d4fe0trac #11840 details, making sure that tests pass

comment:5 Changed 8 years ago by mmezzarobba

  • 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 git

  • Commit changed from 06d4fe0e2b1c40cbebd4f72ffd171ba9b2389db6 to 2b111097f96020a082f669ef426e999bd68597e7

Branch pushed to git repo; I updated commit sha1. New commits:

​2b11109trac #11840 swapped two lines

comment:7 Changed 8 years ago by chapoton

Done. Many thanks for your reviews, here and in other tickets !

comment:8 Changed 8 years ago by mmezzarobba

  • Status changed from needs_review to positive_review

comment:9 Changed 8 years ago by vbraun

Conflict, please merge in latest beta

comment:10 Changed 8 years ago by chapoton

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 vbraun

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 mmezzarobba

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/11840-collect_common_factors.

comment:13 Changed 8 years ago by mmezzarobba

Wait, this change will also conflict with #15892, which probably should go in first.

comment:14 Changed 8 years ago by vbraun

Please merge in 6.2.beta4

comment:15 Changed 8 years ago by git

  • 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:

​f7c9326Merge branch 'u/chapoton/11840' of trac.sagemath.org:sage into 11840

comment:16 Changed 8 years ago by chapoton

merging done

comment:17 Changed 8 years ago by rws

  • 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:

​cedda60Merge branch 'u/chapoton/11840' of git://trac.sagemath.org/sage into tmp
​fb4ca11trac #11840: reviewer's patch: consistent use of imperative definition

comment:18 Changed 8 years ago by rws

  • Reviewers changed from Marc Mezzarobba to Marc Mezzarobba, Ralf Stephan

comment:19 Changed 8 years ago by chapoton

  • 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 vbraun

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