Add numpy.logaddexp to openvino backend #21043
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21043 +/- ##
==========================================
- Coverage 82.47% 82.44% -0.04%
==========================================
Files 563 563
Lines 53834 53857 +23
Branches 8359 8362 +3
==========================================
+ Hits 44401 44402 +1
- Misses 7392 7414 +22
Partials 2041 2041
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
keras/src/backend/openvino/numpy.py
Outdated
| x1 = get_ov_output(x1, element_type) | ||
| x2 = get_ov_output(x2, element_type) | ||
| x1, x2 = _align_operand_types(x1, x2, "logaddexp()") | ||
| x_type = x1.get_element_type() | ||
| if x_type.is_integral(): | ||
| ov_type = OPENVINO_DTYPES[config.floatx()] | ||
| x1 = ov_opset.convert(x1, ov_type) | ||
| x2 = ov_opset.convert(x2, ov_type) | ||
| exp_x1 = ov_opset.exp(x1).output(0) | ||
| exp_x2 = ov_opset.exp(x2).output(0) | ||
| sum_exp = ov_opset.add(exp_x1, exp_x2).output(0) |
There was a problem hiding this comment.
I propose to make it more numerical stable for big x1 and x2 numbers:
ln(e^x1 + e^x2) = ln (e^k(e^(x1-k) + e^(x2-k))) = k ln (e^(x1-k) + e^(x2-k)), where k is maximum from x1 and x2. pay attention that by this trick x1-k and x2-k will be less than one that provides more compute stable for exponent that outputs non-negative value less than one that is better represented for floating point.
There was a problem hiding this comment.
Good catch on compute stability. I have made the changes
| @@ -848,9 +848,23 @@ def log2(x): | |||
|
|
|||
There was a problem hiding this comment.
please switch on tests for this functionality.
There was a problem hiding this comment.
In issue you mentioned ./keras/src/ops/numpy_test.py for testing. I initially thought that tests automatically invoke the numpy from correct backend. But on tracking knp imports in numpy_test file they seem to come from ./keras/src/backend/numpy module instead of openvino. I verify this by executing tests without implementing logaddexp() and they PASSED (a number of other unimplemented functions inside ./keras/src/backend/openvino/numpy.py also pass). What should I change exactly to test the function in current test setup?
There was a problem hiding this comment.
please rebase to latest master and you will see corresponding lines for logaddexp
Adds numpy.logaddexp function to OpenVino backend. Fixes 29489