added log and log1p functions #225
Conversation
| if self.logarithm_function_name is NotImplemented: | ||
| raise NotImplementedError("Logarithm function is not provided") |
There was a problem hiding this comment.
Unfortunately, I didn't find anything that we could use as a fallback expression for LogExpr. All methods are based on reduction technique which requires frexp implementation.
Refer for example to https://golang.org/src/math/log.go
| self.logarithm_function_name, nested_result) | ||
|
|
||
| def interpret_log1p_expr(self, expr, **kwargs): | ||
| self.with_math_module = True |
There was a problem hiding this comment.
I believe we should set this flag only in case when we don't follow the fallback path. In case of the fallback we technically don't require the math module, although subsequent expressions that constitute the fallback expression may choose to enable it.
There was a problem hiding this comment.
I can't believe you ported the log1p function to all those languages
Thanks! Actually it was not so hard. All imperative languages are so similar! 😄
I believe we should set this flag only in case when we don't follow the fallback path.
Sorry, I'm not sure I fully understood you. Do you mean the following?
if self.log1p_function_name is NotImplemented:
return self._do_interpret(
fallback_expressions.log1p(expr.expr), **kwargs)
self.with_math_module = True
If so, it seems reasonable for me. But we should double check cases when fallbacks use functions from math module.
Anyway, I believe that this should be addressed in a separate PR because all other expressions follow the same pattern for now.
There was a problem hiding this comment.
Yeah, this is exactly what I mean!
But we should double check cases when fallbacks use functions from math module.
Yep, but those functions will also be interpreted and the flag will be set accordingly if needed.
Anyway, I believe that this should be addressed in a separate PR because all other expressions follow the same pattern for now.
I'm fine with that. We may want to address this for other functions as well.
There was a problem hiding this comment.
Thanks! I created new issue for that to not forget: #246.
|
@StrikerRUS I believe we now have to add |
|
Thanks a lot for the reminder! Done in a6d2046. |
log1pfunction is needed for #221.I extracted this part into separate PR to not overcomplicate original one.
Custom implementations of
log1pare based onhttps://github.com/statgen/topmed_freeze3_calling/blob/master/vt/lib/Rmath/chebyshev.c
https://hackage.haskell.org/package/statistics-0.8.0.0/docs/src/Statistics-Math.html