SymbolicML / DynamicExpressions.jl

Ridiculously fast symbolic expressions
https://symbolicml.org/DynamicExpressions.jl/dev
Apache License 2.0
103 stars 15 forks source link

Safer way of extending user-defined operators #8

Closed MilesCranmer closed 1 year ago

MilesCranmer commented 1 year ago

This PR makes it safer to extend user-defined operators. Now, you can extend operators like so:

using DynamicExpressions

my_op(x::String, y::String) = x * y

operators = GenericOperatorEnum(; binary_operators=[my_op])

@extend_operators operators

x1 = Node(String; feature=1)
tree = my_op(x1, " World!")

tree(["Hello"])
# Hello World!

This macro will take into account what module called the macro, and define it in the same scope. Thus, it will be much safer than the current method which is a bit of a hack. This new approach is a single macro which spits out a bunch of functions like so:

function Base.:(+)(l::Node{T}, r::Node{T}) where {T}
    return Node(1, l, r)
end

(assuming that + is the first operator).

The downside is that the macro is very complex.


It works for functions defined in modules too:

module A
using DynamicExpressions
function f(x, y)
   x + y ^ 2
end
const operators = OperatorEnum(;binary_operators=[+, f])
@extend_operators operators
end

Then, we can use functions as follows:

julia> Main.A.f
f (generic function with 7 methods)

@odow if you have time, I would be eager to hear your thoughts on this!

MilesCranmer commented 1 year ago

Also, @ChrisRackauckas, I would be interested in hearing what you think of this approach as well.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 3356662301


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/OperatorEnumConstruction.jl 53 65 81.54%
<!-- Total: 53 65 81.54% -->
Totals Coverage Status
Change from base Build 3340998351: -0.7%
Covered Lines: 915
Relevant Lines: 1001

💛 - Coveralls
MilesCranmer commented 1 year ago

Will merge this relatively soon so let me know any other comments. I think while this macro isn't ideal, it's much safer than the current strategy of Base.MainInclude.eval, so should be preferred. We can always improve the macro later; I think the syntax won't need to be changed.

MilesCranmer commented 1 year ago

Going to merge now because I think the current approach of Base.MainInclude.eval is a bit dangerous and I'd like to fix it. However I'm very open to changing this PR's approach in the future - it is by no means final.