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: 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:

Status badges

Description (last modified by slelievre)

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 slelievre

  • Branch set to u/slelievre/map-reduce-cleanup

comment:2 Changed 2 years ago by slelievre

  • Commit set to c6a99cc06cdde03d797b7321d66f8748915a9a50

In preparation for other tasks around map_reduce:


New commits:

c6a99ccFix typos and formatting in map_reduce.py

comment:3 Changed 2 years ago by kdilks

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 variable x). You can even link to sage.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 is reduce(). 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 where simplify_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 and map_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.
Last edited 2 years ago by slelievre (previous) (diff)

comment:4 Changed 2 years ago by git

  • Commit changed from c6a99cc06cdde03d797b7321d66f8748915a9a50 to 6042f07f746ce2199da6f335e011a228c1913f8d

Branch pushed to git repo; I updated commit sha1. New commits:

6042f0727789: address reviewer comments

comment:5 Changed 2 years ago by slelievre

Thanks for the thorough review. I tried to address all your comments, please check.

comment:6 Changed 2 years ago by slelievre

  • Authors set to Samuel Lelièvre
  • Status changed from new to needs_review

Please also add your full name to the "Reviewers" field.

comment:7 Changed 2 years ago by git

  • Commit changed from 6042f07f746ce2199da6f335e011a228c1913f8d to 203062ce0faa29c51d96fd6e93da633b53188915

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

203062c27789: address reviewer comments

comment:8 Changed 2 years ago by slelievre

  • 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 kdilks

  • Reviewers set to Kevin Dilks
  • Status changed from needs_review to positive_review

comment:10 Changed 2 years ago by vbraun

  • Branch changed from u/slelievre/map-reduce-cleanup to 203062ce0faa29c51d96fd6e93da633b53188915
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.