Opened 3 years ago
Closed 3 years ago
#27789 closed enhancement (fixed)
Fix typos and formatting in map_reduce
Reported by: | slelievre | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-8.8 |
Component: | combinatorics | Keywords: | map_reduce |
Cc: | slelievre | Merged in: | |
Authors: | Samuel Lelièvre | Reviewers: | Kevin Dilks |
Report Upstream: | N/A | Work issues: | |
Branch: | 203062c (Commits, GitHub, GitLab) | Commit: | 203062ce0faa29c51d96fd6e93da633b53188915 |
Dependencies: | Stopgaps: |
Description (last modified by )
In src/sage/parallel/map_reduce.py
,
- fix typos and formatting
- minor rewording
- use polynomial variables instead of symbolic variables
Change History (10)
comment:1 Changed 3 years ago by
- Branch set to u/slelievre/map-reduce-cleanup
comment:2 Changed 3 years ago by
- Commit set to c6a99cc06cdde03d797b7321d66f8748915a9a50
comment:3 Changed 3 years ago by
Checked the built documentation and tested, here are my comments:
- There are some lambda functions early on that don't have the semicolon in the right spot.
- In "For this, take a Map function that associates..." - why is "Map" capitalized?
- I think most conventions for sigma notation leave out the "i=" in the superscript. Only comes up in one changed line, but appears a few other places.
- The "x-factorial" isn't standard terminology, it's always "q-factorial". I'd suggest either changing the variable to
q
, or saying that this is the q-factorial (in the variablex
). You can even link tosage.combinat.q_analogues.q_factorial
.
- The associated test also fails. Since
x
is previously defined as the generator of a polynomial ring, Sage will take an expression like(1-x^i)/(1-x)
and create it in an associated fraction field. Here, the method you would use isreduce()
. Except this is a method that just updates the internal representation of the fraction field element and won't return anything, so you'd need to do
sage: A = (prod((1-x**i)/(1-x) for i in range(1, 6))).reduce(); A
The alternative is to do something to clear
x
and have it return to being a formal parameter, and then(1-x^i)/(1-x)
will be treated as a more generic rational expression wheresimplify_rational()
will work.
- Broken doclinks:
profile.profile
,sage.parallel.map_reduce.logger
, a bunch of things that look similar to:meth:`master._shutdown`
- I don't think "Active Tasks" should be capitalized in the
ActiveTaskCounterDarwin
class docstring.
- The last part of this line
during normal usage. Most of the time one should leave it to ``True``,
I think should be
leave it as ``True``
, or a little more awkwardly,leave it set to ``True''
.
- Pretty sure that unpaired right parenthesis should be a comma in
During normal time, that is when all workers are active) a worker ``W`` is
post_process
andmap_function
should have their input as single element list (or something matching the rest of the conventions chosen).
random_worker
docstring has a period at the end.
comment:4 Changed 3 years ago by
- Commit changed from c6a99cc06cdde03d797b7321d66f8748915a9a50 to 6042f07f746ce2199da6f335e011a228c1913f8d
Branch pushed to git repo; I updated commit sha1. New commits:
6042f07 | 27789: address reviewer comments
|
comment:5 Changed 3 years ago by
Thanks for the thorough review. I tried to address all your comments, please check.
comment:6 Changed 3 years ago by
- Status changed from new to needs_review
Please also add your full name to the "Reviewers" field.
comment:7 Changed 3 years ago by
- Commit changed from 6042f07f746ce2199da6f335e011a228c1913f8d to 203062ce0faa29c51d96fd6e93da633b53188915
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
203062c | 27789: address reviewer comments
|
comment:8 Changed 3 years ago by
- Description modified (diff)
Sorry, I had left out one of the reviewer comments, I did a force push, hope that's okay.
comment:9 Changed 3 years ago by
- Reviewers set to Kevin Dilks
- Status changed from needs_review to positive_review
comment:10 Changed 3 years ago by
- Branch changed from u/slelievre/map-reduce-cleanup to 203062ce0faa29c51d96fd6e93da633b53188915
- Resolution set to fixed
- Status changed from positive_review to closed
In preparation for other tasks around
map_reduce
:New commits:
Fix typos and formatting in map_reduce.py