Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543333002:
Added log-domain summation (Closed)
DescriptionAdded log-domain summation
BUG=
TBR=stgao@chromium.org
Committed: https://chromium.googlesource.com/infra/infra/+/12ec7322c19954318ae7426cd85b062b6ecab83d
Patch Set 1 : removing todos #
Total comments: 4
Patch Set 2 : rebase #Patch Set 3 : adding docstrings for tests #Patch Set 4 : rebase #
Total comments: 4
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 21 (10 generated)
Description was changed from ========== Added log-domain summation BUG= ========== to ========== Added log-domain summation BUG= TBR=stgao@chromium.org ==========
wrengr@chromium.org changed reviewers: + katesonia@chromium.org, mbarbella@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
PTAL
Just some minor nits. Seems fine overall. https://codereview.chromium.org/2543333002/diff/40001/appengine/findit/libs/m... File appengine/findit/libs/math/logarithms.py (right): https://codereview.chromium.org/2543333002/diff/40001/appengine/findit/libs/m... appengine/findit/libs/math/logarithms.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. Please run gpylint on these files. https://codereview.chromium.org/2543333002/diff/40001/appengine/findit/libs/m... File appengine/findit/libs/math/test/logarithms_test.py (right): https://codereview.chromium.org/2543333002/diff/40001/appengine/findit/libs/m... appengine/findit/libs/math/test/logarithms_test.py:13: def testLogsumexpEmpty(self): Docstrings here as well.
https://codereview.chromium.org/2543333002/diff/40001/appengine/findit/libs/m... File appengine/findit/libs/math/logarithms.py (right): https://codereview.chromium.org/2543333002/diff/40001/appengine/findit/libs/m... appengine/findit/libs/math/logarithms.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/12/02 20:07:02, Martin Barbella wrote: > Please run gpylint on these files. Done. https://codereview.chromium.org/2543333002/diff/40001/appengine/findit/libs/m... File appengine/findit/libs/math/test/logarithms_test.py (right): https://codereview.chromium.org/2543333002/diff/40001/appengine/findit/libs/m... appengine/findit/libs/math/test/logarithms_test.py:13: def testLogsumexpEmpty(self): On 2016/12/02 20:07:02, Martin Barbella wrote: > Docstrings here as well. Done.
lgtm (still going to need someone else to stamp it, though)
lgtm
The CQ bit was checked by wrengr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2547133002 Patch 60001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by wrengr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480726485389270,
"parent_rev": "eb772e3b3bdf3a166038f66a1e9d14260e1628c6", "commit_rev":
"12ec7322c19954318ae7426cd85b062b6ecab83d"}
Message was sent while issue was closed.
Description was changed from ========== Added log-domain summation BUG= TBR=stgao@chromium.org ========== to ========== Added log-domain summation BUG= TBR=stgao@chromium.org Committed: https://chromium.googlesource.com/infra/infra/+/12ec7322c19954318ae7426cd85b0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/infra/infra/+/12ec7322c19954318ae7426cd85b0...
Message was sent while issue was closed.
inferno@chromium.org changed reviewers: + inferno@chromium.org
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... File appengine/findit/libs/math/logarithms.py (right): https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... appengine/findit/libs/math/logarithms.py:12: """Efficiently and accurately compute a log-domain sum. Where is this idea based off on ? Can you add a wiki link or point to code ? https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... File appengine/findit/libs/math/test/logarithms_test.py (right): https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... appengine/findit/libs/math/test/logarithms_test.py:15: def testLogsumexpEmpty(self): Can we have a test actual value for logsumpexp for simple inputs ? Also, a test that shows different results from math.log(math.fsum(math.exp(x) for x in xs))) ?
Message was sent while issue was closed.
https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... File appengine/findit/libs/math/logarithms.py (right): https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... appengine/findit/libs/math/logarithms.py:12: """Efficiently and accurately compute a log-domain sum. On 2016/12/04 23:51:24, inferno wrote: > Where is this idea based off on ? Can you add a wiki link or point to code ? This is common knowledge in the machine learning community. I can provide a link to when I implemented it in Haskell if you'd like, though it just does the same thing as here. https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... File appengine/findit/libs/math/test/logarithms_test.py (right): https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... appengine/findit/libs/math/test/logarithms_test.py:15: def testLogsumexpEmpty(self): On 2016/12/04 23:51:24, inferno wrote: > Can we have a test actual value for logsumpexp for simple inputs ? > Also, a test that shows different results from math.log(math.fsum(math.exp(x) > for x in xs))) ? Testing concrete values is problematic because the result will be platform & Python-version dependent, and therefore flaky. Showing the result is different from the naive implementation will be similarly flaky for any interesting (i.e., borderline) values, but I can come up with some non-flaky examples where the naive thing always over-/underflows, if desired.
Message was sent while issue was closed.
On 2016/12/05 19:12:59, wrengr wrote: > https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... > File appengine/findit/libs/math/logarithms.py (right): > > https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... > appengine/findit/libs/math/logarithms.py:12: """Efficiently and accurately > compute a log-domain sum. > On 2016/12/04 23:51:24, inferno wrote: > > Where is this idea based off on ? Can you add a wiki link or point to code ? > > This is common knowledge in the machine learning community. I can provide a link > to when I implemented it in Haskell if you'd like, though it just does the same > thing as here. Yes please. > > https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... > File appengine/findit/libs/math/test/logarithms_test.py (right): > > https://codereview.chromium.org/2543333002/diff/100001/appengine/findit/libs/... > appengine/findit/libs/math/test/logarithms_test.py:15: def > testLogsumexpEmpty(self): > On 2016/12/04 23:51:24, inferno wrote: > > Can we have a test actual value for logsumpexp for simple inputs ? > > Also, a test that shows different results from math.log(math.fsum(math.exp(x) > > for x in xs))) ? > > Testing concrete values is problematic because the result will be platform & > Python-version dependent, and therefore flaky. Showing the result is different > from the naive implementation will be similarly flaky for any interesting (i.e., > borderline) values, but I can come up with some non-flaky examples where the > naive thing always over-/underflows, if desired. Yes, that would be good, basically any input values which are more predictable and usuable across platform. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
