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)
Change History (13)
comment:1 Changed 7 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 7 years ago by
comment:3 in reply to: ↑ 2 Changed 7 years ago by
comment:4 Changed 7 years ago by
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: ↓ 6 Changed 7 years ago by
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
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
comment:7 Changed 7 years ago by
apply trac_13727_dict_addition_doc-cs.patch
comment:8 Changed 7 years ago by
- Status changed from needs_review to positive_review
ok, good for me. Positive review
comment:9 Changed 7 years ago by
- Reviewers set to Frédéric Chapoton
comment:10 follow-up: ↓ 11 Changed 7 years ago by
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
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
- Merged in set to sage-5.6.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
There is a typo "the the" in the documentation (at least twice)