Use functions returning NaN on branch cuts instead of abs (issue #109) #123
Conversation
…ts (issue MilesCranmer#109) This commit also removes the regularization terms present in the logs. This is a first step in resolving issue MilesCranmer#109.
The new tests does not only test a point within the domain, but also outside of the domain making sure it returns a NaN.
The test is slightly modified to test the new operators within their domain.
This is to avoid printing e.g. `log_nan` when `log` is sufficient.
This also removes the regularizing '1' that was present in the old definition. Instead the test domain is shifted.
This node is to compensate that the new power-operation does not take the absolute value of the argument. This new pow_nan made the old "tree_bad"run into NaN value producing infinite losses.
There is now a function that uses a dictionary to rename the internal names of the operators to more user friendly ones. This is a bit more strict, and will not rename user-defined functions.
…perators There are now tests for the new safe operators that verifies that the "safe_" part of the name is removed when printed to the user. The tests are not only for the unary operators but the binary "safe_pow" as well.
|
This last batch of commits should hopefully be enough for the review comments (do I press the "Resolve Conversation" buttons for the ones I think I have solved?). That is, I only left the first review question unresolved, and I don't think there is a particular useful way of resolving it. Let me reiterate my issue there: If the resulting expression found is, say, |
|
Thanks, will take a look! Yes - you hit "Resolve Conversation" when a suggestion is addressed. I already clicked them though. |
| - `binary_operators`: Tuple of binary operators to use. Each operator should | ||
| be defined for two input scalars, and one output scalar. All operators | ||
| need to be defined over the entire real line (excluding infinity - these | ||
| are stopped before they are input), or return `NaN` where not defined. |
There was a problem hiding this comment.
Nice, thanks for adding this!
| @@ -31,3 +31,19 @@ EquationSearch( | |||
| s = repr(tree) | |||
| true_s = "((sin(cos(sin(cos(v1) * v3) * 3.0) * -0.5) + 2.0) * 5.0)" | |||
| @test s == true_s | |||
|
|
|||
| for unaop in [safe_log, safe_log2, safe_log10, safe_log1p, safe_sqrt, safe_acosh] | |||
This PR is intended to resolve issue #109 . It replaces logarithmic
_abs,acosh_abs, andsqrt_abswith corresponding functions returningNaNon their corresponding branch cuts. This also removes all regularizing terms in the arguments of the previous functions, e.g. the1//100000000.The corresponding functions for generic types are now mapped to non-regularized functions as well. As I understand the need for these function definitions is to map them to symbolic expressions for the package
SymbolicUtils.jl. The symbolic expression the user gets may have branch cuts, but I think it should be up to the user to analyse those properties of the final expression instead of having the functions mapped to regularized versions of the same functions. But let me know if I'm missing some reason why the symbolic expressions should be regularized. There appear to be no unit-tests for this, let me know if some should be added.The old tests have also been updated: the
test_tree_constructiontests are for the new functions only tested in their valid domain, andtest_operatorstest points inside and outside the domain of the new functions.To note, since
pow_absis marked with@deprecateI did not bother to change this operation. Let me know if there is something missing that should be fixed in relation to power expressions.All tests pass on my computer, and I also did some searches that finished without any apparent errors.