Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2547133002:
Added n-ary vector summation (Closed)
DescriptionAdded n-ary vector summation
BUG=
TBR=stgao@chromium.org
Committed: https://chromium.googlesource.com/infra/infra/+/b17938f2101a3ed9c608d058a0a65da1984dc1f3
Patch Set 1 #
Total comments: 9
Patch Set 2 : addressing nits #Patch Set 3 : Adding docstrings for tests #Patch Set 4 : rebase #Patch Set 5 : removed todo #
Total comments: 6
Messages
Total messages: 21 (8 generated)
Description was changed from ========== Added n-ary vector summation BUG= TBR=stgao@chromium.org ========== to ========== Added n-ary vector summation BUG= TBR=stgao@chromium.org ==========
wrengr@chromium.org changed reviewers: + katesonia@chromium.org, mbarbella@chromium.org
PTAL
https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... File appengine/findit/libs/math/test/vectors_test.py (right): https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... appengine/findit/libs/math/test/vectors_test.py:14: class VectorsTest(unittest.TestCase): 2 blank lines before this one. Does gpylint complain about anything else here? https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... appengine/findit/libs/math/test/vectors_test.py:29: def testVsumKeepsPrecision(self): Please add docstrings for all tests. They're pretty handy in the event of failures (unittest recognizes them). https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... File appengine/findit/libs/math/vectors.py (right): https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... appengine/findit/libs/math/vectors.py:31: # TODO(wrengr): reimplement this in C, inling CPython's implementation FWIW, I don't think that this is a worthwhile goal. It would be too significant a maintenance burden for the win it provides.
wrengr@chromium.org changed reviewers: - stgao@chromium.org
nits addressed https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... File appengine/findit/libs/math/test/vectors_test.py (right): https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... appengine/findit/libs/math/test/vectors_test.py:14: class VectorsTest(unittest.TestCase): On 2016/12/02 19:52:37, Martin Barbella wrote: > 2 blank lines before this one. Does gpylint complain about anything else here? It really wants the numpy import to be grouped with the libs.* one... which seems wrong. Also complains about the copyright header not saying Google Inc https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... appengine/findit/libs/math/test/vectors_test.py:29: def testVsumKeepsPrecision(self): On 2016/12/02 19:52:37, Martin Barbella wrote: > Please add docstrings for all tests. They're pretty handy in the event of > failures (unittest recognizes them). None of the other tests in the repo have them, but whatevs. Done.
lgtm (still need someone else to stamp it) https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... File appengine/findit/libs/math/test/vectors_test.py (right): https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... appengine/findit/libs/math/test/vectors_test.py:14: class VectorsTest(unittest.TestCase): On 2016/12/02 20:54:46, wrengr wrote: > On 2016/12/02 19:52:37, Martin Barbella wrote: > > 2 blank lines before this one. Does gpylint complain about anything else here? > > It really wants the numpy import to be grouped with the libs.* one... which > seems wrong. Also complains about the copyright header not saying Google Inc Yeah, those are fine to ignore. stgao, katesonia: Do we have any presubmit rules related to this? Might be useful if not, and it would also allow you to ignore the rules we don't care about. https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... appengine/findit/libs/math/test/vectors_test.py:29: def testVsumKeepsPrecision(self): On 2016/12/02 20:54:46, wrengr wrote: > On 2016/12/02 19:52:37, Martin Barbella wrote: > > Please add docstrings for all tests. They're pretty handy in the event of > > failures (unittest recognizes them). > > None of the other tests in the repo have them, but whatevs. Done. Thanks. They're really helpful. We probably should have had them on the other ones, but let's at least try to add them for new ones.
lgtm https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... File appengine/findit/libs/math/vectors.py (right): https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... appengine/findit/libs/math/vectors.py:31: # TODO(wrengr): reimplement this in C, inling CPython's implementation On 2016/12/02 19:52:37, Martin Barbella wrote: > FWIW, I don't think that this is a worthwhile goal. It would be too significant > a maintenance burden for the win it provides. +1
https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... File appengine/findit/libs/math/test/vectors_test.py (right): https://codereview.chromium.org/2547133002/diff/1/appengine/findit/libs/math/... appengine/findit/libs/math/test/vectors_test.py:14: class VectorsTest(unittest.TestCase): On 2016/12/02 22:12:11, Martin Barbella wrote: > On 2016/12/02 20:54:46, wrengr wrote: > > On 2016/12/02 19:52:37, Martin Barbella wrote: > > > 2 blank lines before this one. Does gpylint complain about anything else > here? > > > > It really wants the numpy import to be grouped with the libs.* one... which > > seems wrong. Also complains about the copyright header not saying Google Inc > > Yeah, those are fine to ignore. > > stgao, katesonia: Do we have any presubmit rules related to this? Might be > useful if not, and it would also allow you to ignore the rules we don't care > about. No, somehow the presubmit won't check the 2 empty lines before class/function. I guess we need to modify some git tools in infra repo, though I am not quite sure how to add those rules.
The CQ bit was checked by wrengr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from katesonia@chromium.org, mbarbella@chromium.org Link to the patchset: https://codereview.chromium.org/2547133002/#ps80001 (title: "removed todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/02 22:26:12, Sharu Jiang wrote: > On 2016/12/02 22:12:11, Martin Barbella wrote: > > stgao, katesonia: Do we have any presubmit rules related to this? Might be > > useful if not, and it would also allow you to ignore the rules we don't care > > about. > > No, somehow the presubmit won't check the 2 empty lines before class/function. I > guess we need to modify some git tools in infra repo, though I am not quite sure > how to add those rules. Re getting precommit to automatically check for things, gpylint also returns a whole slew of complaints about indentation and the formatting of Args/Returns sections which are at odds with the style we use.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480720940831460,
"parent_rev": "ba0044d3eac27f216508c461ca615436f3892b2f", "commit_rev":
"b17938f2101a3ed9c608d058a0a65da1984dc1f3"}
Message was sent while issue was closed.
Description was changed from ========== Added n-ary vector summation BUG= TBR=stgao@chromium.org ========== to ========== Added n-ary vector summation BUG= TBR=stgao@chromium.org Committed: https://chromium.googlesource.com/infra/infra/+/b17938f2101a3ed9c608d058a0a65... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/infra/infra/+/b17938f2101a3ed9c608d058a0a65...
Message was sent while issue was closed.
On 2016/12/02 23:29:09, wrengr wrote: > On 2016/12/02 22:26:12, Sharu Jiang wrote: > > On 2016/12/02 22:12:11, Martin Barbella wrote: > > > stgao, katesonia: Do we have any presubmit rules related to this? Might be > > > useful if not, and it would also allow you to ignore the rules we don't care > > > about. > > > > No, somehow the presubmit won't check the 2 empty lines before class/function. > I > > guess we need to modify some git tools in infra repo, though I am not quite > sure > > how to add those rules. > > Re getting precommit to automatically check for things, gpylint also returns a > whole slew of complaints about indentation and the formatting of Args/Returns > sections which are at odds with the style we use. This is what I meant by "it would also allow you to ignore the rules we don't care about." We do a lot of that in CF. I'll try to dig a bit more into what we're doing here when I have some free time.
Message was sent while issue was closed.
inferno@chromium.org changed reviewers: + inferno@chromium.org
Message was sent while issue was closed.
lgtm with nits. https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... File appengine/findit/libs/math/test/vectors_test.py (right): https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/math/test/vectors_test.py:21: def testFsumKeepsPrecision(self): Should a test be there for testing precision with BIG for testFsumKeepsPrecision and testVsumKeepsPrecision https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... File appengine/findit/libs/math/vectors.py (right): https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/math/vectors.py:10: """Accurate summation of a list of vectors. Where is this implementation based off ? Can you add wiki link or point to source. https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/math/vectors.py:35: # cost for the performance benefit. typo: of a maintenance
Message was sent while issue was closed.
https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... File appengine/findit/libs/math/test/vectors_test.py (right): https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/math/test/vectors_test.py:21: def testFsumKeepsPrecision(self): On 2016/12/05 00:12:37, inferno wrote: > Should a test be there for testing precision with BIG for testFsumKeepsPrecision > and testVsumKeepsPrecision I don't know what you mean. As math.fsum ships with Python, there's no point in testing it for itself; this test (testFsumKeepsPrecision) is a meta-test to ensure that testVsumKeepsPrecision is valid as a test of our code. That is, if we pick some self._xs which causes even math.fsum to fail to keep precision, then vsum has no hope of doing any better. The key aspect of both tests is that if we evaluate (BIG + LITTLE - BIG) naively then we will get an answer of zero because the intermediate value (BIG + LITTLE) looses precision and its absolute difference against BIG is zero. (FWIW, reordering the summands to the order (LITTLE + BIG - BIG) would achieve the same result.) Conversely, testing (BIG + LITTLE - LITTLE) == BIG doesn't achieve the same purpose. If the intermediate value (BIG + LITTLE) rounds to BIG that's as expected. https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... File appengine/findit/libs/math/vectors.py (right): https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/math/vectors.py:10: """Accurate summation of a list of vectors. On 2016/12/05 00:12:37, inferno wrote: > Where is this implementation based off ? Can you add wiki link or point to > source. This implementation is a gross hack to compute the sums element by element, since mbarbella@ et al thought it wouldn't be worth the maintenance cost to actually vectorize Shewchuk's algorithm. There is no source to link; though I can provide links for where numpy said they WontFix their not providing this function. https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/math/vectors.py:35: # cost for the performance benefit. On 2016/12/05 00:12:37, inferno wrote: > typo: of a maintenance Done.
Message was sent while issue was closed.
On 2016/12/05 19:06:43, wrengr wrote: > https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... > File appengine/findit/libs/math/test/vectors_test.py (right): > > https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... > appengine/findit/libs/math/test/vectors_test.py:21: def > testFsumKeepsPrecision(self): > On 2016/12/05 00:12:37, inferno wrote: > > Should a test be there for testing precision with BIG for > testFsumKeepsPrecision > > and testVsumKeepsPrecision > > I don't know what you mean. > > As math.fsum ships with Python, there's no point in testing it for itself; this > test (testFsumKeepsPrecision) is a meta-test to ensure that > testVsumKeepsPrecision is valid as a test of our code. That is, if we pick some > self._xs which causes even math.fsum to fail to keep precision, then vsum has no > hope of doing any better. The key aspect of both tests is that if we evaluate > (BIG + LITTLE - BIG) naively then we will get an answer of zero because the > intermediate value (BIG + LITTLE) looses precision and its absolute difference > against BIG is zero. (FWIW, reordering the summands to the order (LITTLE + BIG - > BIG) would achieve the same result.) > > Conversely, testing (BIG + LITTLE - LITTLE) == BIG doesn't achieve the same > purpose. If the intermediate value (BIG + LITTLE) rounds to BIG that's as > expected. > > https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... > File appengine/findit/libs/math/vectors.py (right): > > https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... > appengine/findit/libs/math/vectors.py:10: """Accurate summation of a list of > vectors. > On 2016/12/05 00:12:37, inferno wrote: > > Where is this implementation based off ? Can you add wiki link or point to > > source. > > This implementation is a gross hack to compute the sums element by element, > since mbarbella@ et al thought it wouldn't be worth the maintenance cost to > actually vectorize Shewchuk's algorithm. There is no source to link; though I > can provide links for where numpy said they WontFix their not providing this > function. Yes, link to that and add comment. > > https://codereview.chromium.org/2547133002/diff/80001/appengine/findit/libs/m... > appengine/findit/libs/math/vectors.py:35: # cost for the performance benefit. > On 2016/12/05 00:12:37, inferno wrote: > > typo: of a maintenance > > Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
