Minor improvements for dict_addition

sagemath / sage

Main repository of SageMath

https://www.sagemath.org

Other

1.32k stars 453 forks source link

Minor improvements for dict_addition #13727

Closed stumpc5 closed 11 years ago

stumpc5 commented 11 years ago

improvement of the documentation of dict_addition (and also some slight speed gain).

CC: @fchapoton

Component: combinatorics

Keywords: dict, addition

Author: Christian Stump

Reviewer: Frédéric Chapoton

Merged: sage-5.6.beta1

Issue created by migration from https://trac.sagemath.org/ticket/13727

fchapoton commented 11 years ago

comment:2

There is a typo "the the" in the documentation (at least twice)

stumpc5 commented 11 years ago

comment:3

Replying to @fchapoton:

There is a typo "the the" in the documentation (at least twice)

fixed!

fchapoton commented 11 years ago

comment:4

The patch looks good. I have some minor comments on the documentation :

the sentences " a dictionary containing all keys of dictionaries in dict_list (and non-zero values) being the sum of the values in the different dictionaries. "

are not extra-clear. Maybe something like that would be more clear :

a dictionary containing all keys of dictionaries in dict_list, with values being the sum of the values in the different dictionaries (keys with zero value are omitted)

if you use :param x:, maybe you need to write r""" at the beginning of the doc ?

fchapoton commented 11 years ago

comment:5

when replacing the sentence in dict_linear_combination, you have forgotten to keep "each one first multiplied by the given factor"

stumpc5 commented 11 years ago

comment:6

Replying to @fchapoton:

when replacing the sentence in dict_linear_combination, you have forgotten to keep "each one first multiplied by the given factor"

thanks; fixed!

fchapoton commented 11 years ago

comment:7

Attachment: trac_13727_dict_addition_doc-cs.patch.gz

apply trac_13727_dict_addition_doc-cs.patch

fchapoton commented 11 years ago

comment:8

ok, good for me. Positive review

stumpc5 commented 11 years ago

Reviewer: Frédéric Chapoton

stumpc5 commented 11 years ago

Author: Christian Stump

nthiery commented 11 years ago

comment:10

Hmm, are you sure you want to use :param: rather than the usual INPUT field? I know that the developpers guide advertises both, but :param: is seldom used elsewhere. I let you decide!

Cheers,

stumpc5 commented 11 years ago

comment:11

Replying to @nthiery:

Hmm, are you sure you want to use :param: rather than the usual INPUT field? I know that the developpers guide advertises both, but :param: is seldom used elsewhere. I let you decide!

I saw that Travis used parameters for simplicial complexes - and I must say that I do very much prefer this in the html documentation!

If others say: but most people use the command line, and there "INPUT" is nicer. Then okay, I would revert back to use "INPUT". Otherwise I leave parameters as is.

jdemeyer commented 11 years ago

Merged: sage-5.6.beta1