Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sage.symbolic.expression.Expression.collect_common_factors has no documentation #11840

Closed
orlitzky opened this issue Sep 23, 2011 · 25 comments
Closed

sage.symbolic.expression.Expression.collect_common_factors has no documentation #11840

orlitzky opened this issue Sep 23, 2011 · 25 comments
Assignees
@sagetrac-mvngu
Labels
c: documentation good first issue p: major / 3 t: bug
Milestone
sage-6.2

Comments

@orlitzky
Copy link
Contributor

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.

Component: documentation

Keywords: symbolic

Author: Frédéric Chapoton

Branch/Commit: fb4ca11

Reviewer: Marc Mezzarobba, Ralf Stephan

Issue created by migration from https://trac.sagemath.org/ticket/11840
@orlitzky orlitzky added this to the sage-5.11 milestone Sep 23, 2011
@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@dkrenn
Copy link
Contributor

dkrenn commented Feb 18, 2014

Changed keywords from none to symbolic

@fchapoton
Copy link
Contributor

Commit: 06d4fe0

@fchapoton
Copy link
Contributor

comment:4

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

@fchapoton
Copy link
Contributor

Branch: u/chapoton/11840

@fchapoton
Copy link
Contributor

Author: Frédéric Chapoton

@mezzarobba
Copy link
Member

comment:5

I'd put the comment about LOG_TEN_TWO_PLUS_EPSILON before its definition rather than below. The rest looks fine to me.

@mezzarobba
Copy link
Member

Reviewer: Marc Mezzarobba

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 4, 2014

Changed commit from 06d4fe0 to 2b11109

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 4, 2014

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

2b11109trac #11840 swapped two lines

@fchapoton
Copy link
Contributor

comment:7

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

@vbraun
Copy link
Member

vbraun commented Mar 5, 2014

comment:9

Conflict, please merge in latest beta

@fchapoton
Copy link
Contributor

comment:10

Sorry, but I do not see any conflit with 6.2.beta3. Is there any 6.2.beta4 ?

@vbraun
Copy link
Member

vbraun commented Mar 5, 2014

comment:11

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.

@mezzarobba
Copy link
Member

comment:12

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.

@mezzarobba
Copy link
Member

comment:13

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

@vbraun
Copy link
Member

vbraun commented Mar 20, 2014

comment:14

Please merge in 6.2.beta4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 21, 2014

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 21, 2014

Changed commit from 2b11109 to f7c9326

@fchapoton
Copy link
Contributor

comment:16

merging done

@rwst
Copy link

rwst commented Mar 25, 2014

comment:17

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

@rwst
Copy link

rwst commented Mar 25, 2014

Changed branch from u/chapoton/11840 to public/11840

@rwst
Copy link

rwst commented Mar 25, 2014

Changed commit from f7c9326 to fb4ca11

@rwst
Copy link

rwst commented Mar 25, 2014

Changed reviewer from Marc Mezzarobba to Marc Mezzarobba, Ralf Stephan

@fchapoton
Copy link
Contributor

comment:19

ok, then good to me too. Setting to positive review. Thanks for the review.

@vbraun
Copy link
Member

vbraun commented Apr 1, 2014

Changed branch from public/11840 to fb4ca11

@vbraun vbraun closed this as completed in 1039e08 Apr 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sagetrac-mvngu sagetrac-mvngu

Labels
c: documentation good first issue p: major / 3 t: bug
Projects
None yet
Milestone
sage-6.2
Development

No branches or pull requests
7 participants
@vbraun @rwst @orlitzky @jdemeyer @mezzarobba @fchapoton @dkrenn