Opened 7 years ago

Closed 7 years ago

#13727 closed enhancement (fixed)

Minor improvements for dict_addition

Reported by: stumpc5 Owned by: sage-combinat
Priority: major Milestone: sage-5.6
Component: combinatorics Keywords: dict, addition
Cc: chapoton Merged in: sage-5.6.beta1
Authors: Christian Stump Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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

Attachments (1)

trac_13727_dict_addition_doc-cs.patch (6.7 KB) - added by stumpc5 7 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by stumpc5

  • Status changed from new to needs_review

comment:2 follow-up: Changed 7 years ago by chapoton

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

comment:3 in reply to: ↑ 2 Changed 7 years ago by stumpc5

Replying to chapoton:

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

fixed!

comment:4 Changed 7 years ago by chapoton

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 ?

comment:5 follow-up: Changed 7 years ago by chapoton

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

comment:6 in reply to: ↑ 5 Changed 7 years ago by stumpc5

Replying to chapoton:

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

thanks; fixed!

Changed 7 years ago by stumpc5

comment:7 Changed 7 years ago by chapoton

apply trac_13727_dict_addition_doc-cs.patch

comment:8 Changed 7 years ago by chapoton

  • Status changed from needs_review to positive_review

ok, good for me. Positive review

comment:9 Changed 7 years ago by stumpc5

  • Authors set to Christian Stump
  • Reviewers set to Frédéric Chapoton

comment:10 follow-up: Changed 7 years ago by 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!

Cheers,

comment:11 in reply to: ↑ 10 Changed 7 years ago by stumpc5

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.

comment:12 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.6.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.