Opened 2 years ago
Closed 2 years ago
#27789 closed enhancement (fixed)
Fix typos and formatting in map_reduce
Reported by:  slelievre  Owned by:  

Priority:  minor  Milestone:  sage8.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 2 years ago by
 Branch set to u/slelievre/mapreducecleanup
comment:2 Changed 2 years ago by
 Commit set to c6a99cc06cdde03d797b7321d66f8748915a9a50
comment:3 Changed 2 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 "xfactorial" isn't standard terminology, it's always "qfactorial". I'd suggest either changing the variable to
q
, or saying that this is the qfactorial (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(1x^i)/(1x)
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((1x**i)/(1x) 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(1x^i)/(1x)
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 2 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 2 years ago by
Thanks for the thorough review. I tried to address all your comments, please check.
comment:6 Changed 2 years ago by
 Status changed from new to needs_review
Please also add your full name to the "Reviewers" field.
comment:7 Changed 2 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 2 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 2 years ago by
 Reviewers set to Kevin Dilks
 Status changed from needs_review to positive_review
comment:10 Changed 2 years ago by
 Branch changed from u/slelievre/mapreducecleanup 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