forked from symforce-org/symforce
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[SymForce] Fix singularity in barriers
The derivative of `Pow(x, y)` wrt `x` is implemented both in sympy and symengine (simplified for a constant exponent) as effectively: ``` Pow(x, y) * y / x ``` This is a simplification of the full rule where `x` and `y` can be functions of the independent variable, e.g. from sympy: ``` def _eval_derivative(self, s): from sympy.functions.elementary.exponential import log dbase = self.base.diff(s) dexp = self.exp.diff(s) return self * (dexp * log(self.base) + dbase * self.exp/self.base) ``` SymEngine has a shortcut for when the exponent is a `Number` specifically, although SymPy achieves the same result through automatic simplification: ``` void DiffVisitor::bvisit(const Pow &self) { if (is_a_Number(*(self.get_exp()))) { apply(self.get_base()); result_ = mul( mul(self.get_exp(), pow(self.get_base(), sub(self.get_exp(), one))), result_); } else { apply(mul(self.get_exp(), log(self.get_base()))); result_ = mul(self.rcp_from_this(), result_); } } ``` Of course, the problem is that all of these require dividing by the base, and are therefore singular if the base is 0. You might want to instead formulate this derivative when the exponent is a Number, or more generally an expression whose derivative is 0, as something like `Pow(x, y - 1) * y` instead. I've tested this, and for our uses where we typically need both the value and the derivative, it's more ops and in particular more `pow`s, so we'd rather have the divide. Again, for `Number` exponents, both SymEngine and SymPy simplify this down automatically to the `Pow(x, y - 1) * y` version, it's only actual runtime floating-point Pows that are this way. So, the barrier functions that can use `Pow` this way need to handle this, specifically if the power is not a constant at codegen time. It's usually a bad idea to do this for performance reasons, but it needs to work anyway. Added a test that checks the result is nonsingular. The interesting point to test for epsilon handling would be the transition point, where the derivative doesn't always even exist, so I'm not adding an epsilon handling check. Another thing we could possibly do is have `sf.Pow` take an epsilon? Topic: sf-barrier-pow-epsilon Reviewers: bradley,nathan,philipp,hayk GitOrigin-RevId: 76297a5c73d5845b381a747460948e5d334cd23a
- Loading branch information
1 parent
bdbcbf7
commit 9ec1de7
Showing
4 changed files
with
55 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters