Lambdify as a module level function #61

Merged
merged 14 commits into from Jul 29, 2016

Projects

None yet

4 participants

@rajithv
Contributor
rajithv commented Jul 13, 2016

Uses the 2nd option mentioned in #35

@rajithv rajithv Lambdify as a module level function
a63684d
@zverok zverok and 1 other commented on an outdated diff Jul 13, 2016
lib/symengine.rb
@@ -33,6 +33,19 @@ def Function(n)
def evalf(operand, prec=53, real=false)
return _evalf(operand, prec, real)
end
+ def lambdify(exp)
+ sym_map = exp.free_symbols.map { |sym| sym.to_s}.join(",")
+ str = exp.to_s
+ str.gsub!(/[\d\.]+/, 'Rational(\0,1)')
+ replacements = [
+ ["asin", "Math.asin"], ["acos", "Math.acos"], ["atan","Math.atan"],
+ ["sin", "Math.sin"], ["cos", "Math.cos"], ["tan","Math.tan"],
+ ["asinh", "Math.asinh"], ["acosh", "Math.acosh"], ["atanh","Math.atanh"],
+ ["sinh", "Math.sinh"], ["cosh", "Math.cosh"], ["tanh","Math.tanh"],
+ ["pi", "Math::PI"], ["E", "Math::E"] ]
@zverok
zverok Jul 13, 2016 Collaborator

Better to store it as a constant (and as a hash, like {asin: 'Math.asin', acos: 'Math.acos'.... -- not sure, though, if Symbol keys will suit all needs).

@rajithv
rajithv Jul 13, 2016 Contributor

I will do that, and if we come across a scenario where it doesn't suit, will revert.

@zverok zverok commented on an outdated diff Jul 13, 2016
lib/symengine.rb
@@ -33,6 +33,19 @@ def Function(n)
def evalf(operand, prec=53, real=false)
return _evalf(operand, prec, real)
end
+ def lambdify(exp)
+ sym_map = exp.free_symbols.map { |sym| sym.to_s}.join(",")
+ str = exp.to_s
+ str.gsub!(/[\d\.]+/, 'Rational(\0,1)')
+ replacements = [
+ ["asin", "Math.asin"], ["acos", "Math.acos"], ["atan","Math.atan"],
+ ["sin", "Math.sin"], ["cos", "Math.cos"], ["tan","Math.tan"],
+ ["asinh", "Math.asinh"], ["acosh", "Math.acosh"], ["atanh","Math.atanh"],
+ ["sinh", "Math.sinh"], ["cosh", "Math.cosh"], ["tanh","Math.tanh"],
+ ["pi", "Math::PI"], ["E", "Math::E"] ]
+ replacements.each {|replacement| str.gsub!(/(\b#{replacement[0]}\b)/, replacement[1])}
+ return eval("lambda { |"+ sym_map +"| "+ str +" }")
@zverok
zverok Jul 13, 2016 Collaborator

String interpolation is preferred method of creating complex strings ("lambda { |#{sym_map}| #{str} }"). And return keyword is unnecessary here :)

rajithv added some commits Jul 13, 2016
@rajithv rajithv Tests for lambdify
a91d406
@rajithv rajithv String interpolation for lambda creation
f4ec00a
@rajithv
Contributor
rajithv commented Jul 13, 2016

@zverok I added the specs, I think I did it better this time ;)

If you can please give your comments before I write for all other trig and hyperbolic functions, would appreciate. (I'm assuming it's better to write tests covering all the SymEngine functions to test the mappings etc.)

@zverok
Collaborator

BTW, for such a complicated case, I assume "direct" tests (what exactly is produced) are better than indirect (what will be calculated by that lambda). So, something like lambda_code method could be exposed and tested for exactly right code produced even for most complicated cases.

WDYT?

@rajithv
Contributor
rajithv commented Jul 13, 2016

Yep. I think that's better. Especially considering that we're already sure about the result, given the lambda is correctly generated. I'll get on to it.

rajithv added some commits Jul 14, 2016
@rajithv rajithv Added other symengine functions for lambdify
98ef2b5
@rajithv rajithv Added tests for Lambdifying code
9872b75
@zverok
Collaborator
zverok commented on spec/lambdify_spec.rb in 9872b75 Jul 14, 2016 edited

I think you can just do something like

def l(code)
  SymEngine.lambdify_code(code) 
end

before this it, and inside it just

expect(l( x + 5 )).to eq("lambda { | x | Rational(5,1) + x }")

Just less visual garbage, y'know.

Another interesting solution is defining custom matcher (it would be pretty easy for this case), and write code like this:

expect(x + 5).to be_lambdified_to("lambda { | x | Rational(5,1) + x }")
@rajithv rajithv Sorting symbols for lambda in alphabetical order
9e87548
@isuruf isuruf and 2 others commented on an outdated diff Jul 15, 2016
lib/symengine.rb
@@ -33,6 +33,29 @@ def Function(n)
def evalf(operand, prec=53, real=false)
return _evalf(operand, prec, real)
end
+ def evalf_dirichlet_eta(exp)
+ SymEngine::evalf(SymEngine::dirichlet_eta(exp))
+ end
+ def evalf_zeta(exp)
+ SymEngine::evalf(SymEngine::zeta(exp))
+ end
+ def lambdify_code(exp)
+ sym_map = exp.free_symbols.map { |sym| sym.to_s}.sort.join(",")
@isuruf
isuruf Jul 15, 2016 Member

I only noticed this right now. The user should pass a set of variables in an order. Guessing the symbols is not a good way forward.

@rajithv
rajithv Jul 15, 2016 Contributor

Wouldn't that be unintuitive?
Also, wouldn't the free_symbols method always return all the free symbols.. are there cases where it wouldn't?

@isuruf
isuruf Jul 15, 2016 edited Member

You sometimes need to control the order of the variable and this doesn't give you that.
For the 2nd part an eg:

ex = expand((x + y)**2 - 2*x*y-y**2)
l = lambdify(ex)
l.call(1, 2)

User expects in the last call that the expression is evaluated at x=1 and y=2, but this would give an error because there is only one free symbol in ex, which is x

@rajithv
rajithv Jul 15, 2016 Contributor

well, in that case, how about taking an optional parameter of a symbol array. If it is there, the user's symbols can be taken, if not, the free symbols of the expression can be taken. what do you think?

@isuruf
isuruf Jul 15, 2016 Member

Getting a symbol array is a great idea.
Problem with free_symbols is that the order is architecture dependent. For example, in cylinder_volume = SymEngine::PI * SymEngine::Symbol.new(:r) ** 2 * SymEngine::Symbol.new(:h), free_symbols is a set whose order is unspecified in SymEngine and it could be either r, h or h, r depending on your os and toolchain.

So, auto generating the order is going to hurt reproducibility and reproducibility is something we should be careful about.

@zverok
zverok Jul 15, 2016 Collaborator

Problem with free_symbols is that the order is architecture dependent.

Wow. Is it SymEngine's documented behavior?.. It looks ahem, not very smart on their side, ahem.

@rajithv
rajithv Jul 15, 2016 Contributor

I figured that out while testing.. so I wrote it in a way now that the symbols appear in the same order as they appeared in the equation.

@zverok
zverok Jul 15, 2016 Collaborator

I think -- let's create an issue/question in main symengine repository, explaining our case? Maybe it will lead them to somewhat more reasonable design?..

@rajithv
rajithv Jul 15, 2016 Contributor

The above solution didn't work out properly. The order of to_s function is also similar.. it's not in line with the original symbol order.. so we have to limit it to giving a set of symbols

@isuruf isuruf commented on an outdated diff Jul 15, 2016
lib/symengine.rb
@@ -33,6 +33,29 @@ def Function(n)
def evalf(operand, prec=53, real=false)
return _evalf(operand, prec, real)
end
+ def evalf_dirichlet_eta(exp)
+ SymEngine::evalf(SymEngine::dirichlet_eta(exp))
+ end
+ def evalf_zeta(exp)
@isuruf
isuruf Jul 15, 2016 Member

Is it possible to remove these from here like in a sub namespace so that the SymEngine namespace is not polluted?

@isuruf isuruf commented on an outdated diff Jul 15, 2016
spec/lambdify_spec.rb
+ let(:lamb) { SymEngine::lambdify(func) }
+ it 'performs addition with a lambda function' do
+ expect(lamb.call(1)).to eq(6)
+ expect(lamb.call(0)).to eq(5)
+ expect(lamb.call(-1)).to eq(4)
+ expect(lamb.call(Math::PI)).to be_within(0.000000000000001).of(8.141592653589793)
+ end
+ end
+
+ describe 'lambda for sin' do
+ let(:func) { SymEngine::sin(x) }
+ let(:lamb) { SymEngine::lambdify(func) }
+ it 'performs sin calculation with a lambda function' do
+ expect(lamb.call(0)).to be_within(0.000000000000001).of(0.0)
+ expect(lamb.call(Math::PI)).to be_within(0.000000000000001).of(0.0)
+ expect(lamb.call(Math::PI/2)).to be_within(0.000000000000001).of(1.0)
@isuruf
isuruf Jul 15, 2016 Member

Use 1e-15

@rajithv rajithv Restructured to clean up SymEngine module
4f01217
@zverok
Collaborator

Are you sure it is sane?.. By default I'd expect the same order of symbols as in formula:

cylinder_volume = SymEngine::PI * SymEngine::Symbol.new(:r) ** 2 * SymEngine::Symbol.new(:h)
r = 100
h = 20
cylinder_volume.lambdify.call(r, h) # in fact, lambdifies as lambda { |h, r| PI*r**2*h }, and substitutes 100 as h and 20 as r
Collaborator
zverok replied Jul 15, 2016 edited

BTW, one more sudden idea while drawing this example! Why we are calling the thing SymEngine::lambdify, when it should be SymEngine::Basic.to_proc, by all Ruby conventions?..

This way you can do nice things like this:

radii = [10, 20, 30]
formula = SymEngine::PI * SymEngine::Symbol.new(:r) ** 2
radii.map(&formula) # => calculated by formula transparently
Contributor

Yep. I didn't think of that when introducing the sort.

making it to_proc looks like the right thing to do. What do you think @isuruf ?

Contributor

Isn't it good to have both? Retaining lamdify because it's similar to SymEngine's lambdify_double, and having to_proc to please Ruby conventions?

Collaborator
zverok replied Jul 15, 2016 edited

I think it is good, yes (with documentation for lambdify explicitly stating that to_proc is preferred for Ruby)

Member

Both ways are good imo. (Even having only to_proc is fine.)

@rajithv rajithv Fixed the order of symbols, optional symbol set input, to_proc for ba…
…sics
a2ac802
@zverok
Collaborator

What will the code do on (x + y + z).lambdify(:a, :b, :c)?..

@rajithv
Contributor
rajithv commented Jul 15, 2016

@zverok I'm not sure what you meant by, (x + y + z).lambdify(:a, :b, :c)

a lambdify call would be in the form of lam = SymEngine::lambdify(x+y+z, [x, y, z]) and lam.call(:a, :b, :c) would result in an error because NoMethodError: undefined method '+' for :x:Symbol error.

But this can be called with SymEngine::Symbols.

Eg:

a = SymEngine::Symbol.new('a') etc.

lam.call(a, b, c)

Does this look okay?

@rajithv
Contributor
rajithv commented Jul 15, 2016

Also, as I mentioned in an earlier comment, a set of symbols need to be given for lambdify function.
Is it possible to take another input for the .to_proc function, when calling it implicitly in a situation like below?

radii = [10, 20, 30]
formula = SymEngine::PI * SymEngine::Symbol.new(:r) ** 2
radii.map(&formula) # => calculated by formula transparently

Any help on figuring this out?

@rajithv
Contributor
rajithv commented Jul 16, 2016

Sorry for multiple comments. Finally I decided to go ahead with the following:

  1. SymEngine::lambdify(SymEngine::Basic, Array[SymEngine::Symbol])
    Because the only other way to assure the symbol order is to sort it alphabetically, which is not intuitive. So taking the array in as a required parameter.
  2. SymEngine::Basic.to_proc - only for expressions with 1 free symbol. Using to_proc with bigger expressions would not be straight forward, and I don't see a way of passing the symbol array as an argument.
@rajithv rajithv Changed structure of lambdify, added exceptions, altered tests
f871359
@rajithv
Contributor
rajithv commented Jul 18, 2016

@isuruf @zverok review please :)

@zverok
Collaborator
zverok commented Jul 18, 2016

Looking at it.

@zverok zverok commented on an outdated diff Jul 18, 2016
lib/symengine.rb
+ eval(SymEngine::Utils::lambdify_code(exp, syms))
+ end
+ end
+ module Utils
+ class << self
+ def evalf_dirichlet_eta(exp)
+ SymEngine::evalf(SymEngine::dirichlet_eta(exp))
+ end
+ def evalf_zeta(exp)
+ SymEngine::evalf(SymEngine::zeta(exp))
+ end
+ def lambdify_code(exp, syms)
+ str = exp.to_s
+ sym_map = syms.join(",")
+ str.gsub!(/[\d\.]+/, 'Rational(\0,1)')
+ replacements = { sin:"Math.sin", cos: "Math.cos", tan: "Math.tan",
@zverok
zverok Jul 18, 2016 Collaborator

let's make it a module constant.

@zverok zverok commented on an outdated diff Jul 18, 2016
lib/symengine.rb
+ def lambdify(exp, syms)
+ eval(SymEngine::Utils::lambdify_code(exp, syms))
+ end
+ end
+ module Utils
+ class << self
+ def evalf_dirichlet_eta(exp)
+ SymEngine::evalf(SymEngine::dirichlet_eta(exp))
+ end
+ def evalf_zeta(exp)
+ SymEngine::evalf(SymEngine::zeta(exp))
+ end
+ def lambdify_code(exp, syms)
+ str = exp.to_s
+ sym_map = syms.join(",")
+ str.gsub!(/[\d\.]+/, 'Rational(\0,1)')
@zverok
zverok Jul 18, 2016 Collaborator

Are you sure it will cover all possible numbers in sane way?.. It looks a bit hack-ey.

@zverok zverok commented on an outdated diff Jul 18, 2016
lib/symengine.rb
+ end
+ def evalf_zeta(exp)
+ SymEngine::evalf(SymEngine::zeta(exp))
+ end
+ def lambdify_code(exp, syms)
+ str = exp.to_s
+ sym_map = syms.join(",")
+ str.gsub!(/[\d\.]+/, 'Rational(\0,1)')
+ replacements = { sin:"Math.sin", cos: "Math.cos", tan: "Math.tan",
+ asin:"Math.asin", acos: "Math.acos", atan: "Math.atan",
+ sinh:"Math.sinh", cosh: "Math.cosh", tanh: "Math.tanh",
+ asinh:"Math.asinh", acosh: "Math.acosh", atanh: "Math.atanh",
+ pi: "Math::PI", E: "Math::E",
+ dirichlet_eta: "SymEngine::Utils::evalf_dirichlet_eta",
+ zeta: "SymEngine::Utils::evalf_zeta", gamma: "Math.gamma" }
+ replacements.each {|key, value| str.gsub!(/(\b#{key}\b)/, value)}
@zverok
zverok Jul 18, 2016 Collaborator

Better to use inject here instead of modyfying gsub:

str = replacements.inject(str) { |res, (from, to)| res.gsub(/(\b#{from}\b)/, to)}

Overall, modern ruby tends to more "functional" style, without modyfying variables inplace.

Another thing I'm concerned a bit is ineffectiveness of constructing ton of regexes on the fly. Maybe replacements hash could be defined somewhat like:

REPLACEMENTS = {
  asin:"Math.asin", acos: "Math.acos", ....
}.map { |from, to| [/(\b#{from}\b)/, to] }.to_h.freeze
@zverok zverok and 2 others commented on an outdated diff Jul 18, 2016
lib/symengine/basic.rb
@@ -11,8 +11,13 @@ def free_symbols
end
def abs
- return SymEngine::abs(self)
+ SymEngine::abs(self)
+ end
+ def to_proc()
+ if self.free_symbols.count > 1
+ raise ArgumentError, "Too many free symbols! Only 1 allowed. Found #{self.free_symbols.count}."
@zverok
zverok Jul 18, 2016 Collaborator

Not sure we need it:

l = eval("lambda{|x,y,z| x + y + z}")
# => #<Proc:0xa51d510@(eval):1 (lambda)> 
[[1,2,3], [4,5,6]].map(&l)
# => [6, 15] 

Ruby's smart as hell :)

@rajithv
rajithv Jul 20, 2016 Contributor

Actually here the problem was sending arguments to to_proc method.
Because of the ordering problems discussed earlier, sending the symbol array is essential.

Is there a way to send arguments to the to_proc when it's called transparently(?) with the & operator(?).

@zverok
zverok Jul 20, 2016 Collaborator

Aaaah, symbols ordering :( Got it. No, no magical way to add arguments we have, but explicit call may be good enough?..

[[1, 10], [2,20]].map(&formula.to_proc(:h, :r))

So, it looks like #to_proc should accept list of symbols, and rais in case when there is more than one free symbol AND no symbol list passed. WDYT?

@rajithv
rajithv Jul 20, 2016 Contributor

That's also possible. It's basically a matter of better user experience. I didn't want to make users to type in something like [[1, 2], [3,4]].map(&func.to_proc([x, y])) for two argument functions while for 1 argument functions it was [1, 2].map(&func).

imho, it would be simpler if the user just creates a lambda, (lam = SymEngine::lambdify(func, [x, y])) and calls the lambda from there onwards. [[1, 2], [3, 4]].map(&lam)

This looks much clear.

But maybe for those who are more familiar with the ever so complex ruby code, it may be better to allow both :D Let me know what you think, I'll make to_proc work like you described.

@zverok
zverok Jul 20, 2016 edited Collaborator

.to_proc([x, y]) is indeed weird... while .to_proc(:x, :y) for me is clean enough.

In fact, it depends on target usage of SymEngine, we should think about it a bit more (yet, for me as rubyist, func.blah(someshing) is much, much cleaner than SymEngine.blah(func, something) -- if it is definitely "method to convert func into blah")

In addition, I still think it is worth to politely ask main SymEngine engineers to clarify the matters about free symbols order ;)

@rajithv
rajithv Jul 21, 2016 Contributor

Overall, I think for the ruby wrappers, lot more 'rubification' is needed. For example, to support .to_proc(:x, :y) we'd need to map Ruby symbols to SymEngine symbols. Until then I can implement .to_proc(x, y) where x and y are of SymEngine::Symbol type. Will add this as an issue too.

@isuruf
isuruf Jul 21, 2016 edited Member

Can you explain the difficulty with the free symbols order?
FYI, In SymEngine we keep pi*r**2*h as an ordered dictionary. {pi : 1, r : 2, h : 1}. This order is not dependent on which symbol was used first in the formula, pi*r**2*h

@zverok
zverok Jul 21, 2016 Collaborator

@isuruf so, on what the order depends?.. on alphabetical order of symbols?

(And I believe previously it was pointed that symbols order is not guaranteed at all, but I haven't checked it myself)

@isuruf
isuruf Jul 21, 2016 Member

It depends on the hash, but we can order it on alphabetical order on the ruby side.

@zverok
zverok Jul 21, 2016 Collaborator

For the discussed cases, the only reasonable order is order they occur in the formula.
Returning to cylinder volume example:

formula = PI*r**2*h
cylinders = [[10, 100], [20, 200]] # first is radius, second is height
cylinders.map(&formula) # if formula sorts symbols alphabetically, the results would be unexpected
@isuruf
isuruf Jul 21, 2016 Member

SymEngine doesn't track the order in which the formula is created. PI*r**2*h and PI*h*r**2 both have the same representation in SymEngine and are indistinguishable.
cc @certik

@zverok
zverok Jul 21, 2016 Collaborator

OK, then we should stick to explicit symbols list for formulas with more than 1 free symbol. (And consider named arguments option alongside)

@zverok zverok commented on an outdated diff Jul 18, 2016
lib/symengine/basic.rb
@@ -11,8 +11,13 @@ def free_symbols
end
def abs
- return SymEngine::abs(self)
+ SymEngine::abs(self)
+ end
+ def to_proc()
+ if self.free_symbols.count > 1
+ raise ArgumentError, "Too many free symbols! Only 1 allowed. Found #{self.free_symbols.count}."
+ end
+ SymEngine::lambdify(self, self.free_symbols.map {|s| s.to_s})
@zverok
zverok Jul 18, 2016 Collaborator

Why .map {|s| s.to_s}?..

@zverok
Collaborator
zverok commented Jul 18, 2016 edited

Overall, looks pretty good.

One more thing I have in mind is that considering unspecified symbols order problem, for Ruby >= 2.1 we may want to provide an option for named arguments:

(PI * SymEngine(:r) ** 2 * SymEngine(:h)).to_proc(:named) # => lambda { |r:, h:| PI*r**2*h }
(PI * SymEngine(:r) ** 2 * SymEngine(:h)).to_proc(:named).call(r: 10, h: 20)
# and even:
[{h: 1, r: 2}, {h: 3, r: 4}].map(&formula.to_proc(:named)) # => also works!

But this idea can be left for further considerations.

@rajithv rajithv Improvements to lambdify, to_proc
a5221d8
@rajithv
Contributor
rajithv commented Jul 22, 2016

@zverok test for Ruby versions before 2.2 are failing.
In Ruby 2.0, it's because of to_h method.
In Ruby 2.1, the following test fails, expect([[1, 2], [3, 4]].map(&func2.to_proc(x, y))).to eq([13, 17]) I think this is because of the splat arguments I used in the to_proc method. I'm not exactly sure though. (It gives an Argument Error)

For Ruby 2.0 issue, we can use it without to_h right? and for Ruby 2.1 issue, we can have arguments to be sent as an array, without using splat (but this would mean less intuitive code.

Please advice.

@rajithv rajithv Lambdify for i
b45d182
@zverok
Collaborator
zverok commented Jul 23, 2016

In Ruby 2.0, it's because of to_h method.

I think it is almost always a good idea to add backports gem to dependencies and have #to_h in rubies < 2.1. It is pretty safe.

Looking at Ruby 2.1 issue currently.

@zverok
Collaborator
zverok commented Jul 23, 2016

Yeah, it seems lambda splatting behavior was changed in 2.2:

l = lambda {|x,y| x+y}
[[1,2], [3,4]].map(&l)
# => [3, 7] in 2.2, ArgumentError in 2.1

# BUT!
l = proc {|x,y| x+y}
[[1,2], [3,4]].map(&l)
# => [3, 7] in 2.1 too, and even in 1.8.7, the oldest Ruby I have

Differences of proc and lambda is pretty complicated topic, but in our case it does not matter much (except for correct splatting), so I assume you can safely replace it.

@rajithv rajithv lambda-> proc, changing specs, adding backports
850a45d
@zverok
Collaborator

It should be runtime dependency, I believe. Or to_h is used only in specs?..

@rajithv rajithv Changed error messages, minor changes in basic.rb
99bf665
@v0dro
v0dro commented Jul 24, 2016 edited

@abinashmeher999 @isuruf is this ready for merging?

@isuruf isuruf commented on an outdated diff Jul 25, 2016
lib/symengine/basic.rb
@@ -11,8 +11,20 @@ def free_symbols
end
def abs
- return SymEngine::abs(self)
+ SymEngine::abs(self)
+ end
+ def to_proc(*args)
+ if args.empty?
+ if free_symbols.count > 1
+ raise ArgumentError, "You should provide symbols order for #to_proc. Only formulae with 1 free symbol can deduce its name automatically (#{free_symbols.count} found in #{self})."
@isuruf
isuruf Jul 25, 2016 Member

Can you break this code line into 2, so that it doesn't overflow?

@isuruf
Member
isuruf commented Jul 25, 2016

@rajithv, can you split long lines into multiple lines? You can pick a code formatter for ruby and stick with it. @zverok, @v0dro, @abinashmeher999 any suggestions for automatic formatters?

@isuruf
Member
isuruf commented Jul 25, 2016

Formatting should be done, otherwise +1

@zverok
Collaborator
zverok commented Jul 25, 2016

@isuruf we are using rubocop in other SciRuby projects. It is a bit more that just code formatter, but overall code quality checker.

However, introducing it could be problematic it first (it would find tons of errors and warnings, and can automatically fix only code formatting ones, but not logic ones, like method arguments naming, code complexity and so on). Yet, it could generate "todo-config" with all of those warnings, to fix them one by one.

What I am trying to say, "rubocopping" code is a really good thing, but not the most trivial one.

@zverok zverok commented on an outdated diff Jul 25, 2016
lib/symengine.rb
+ class << self
+ REPLACEMENTS = { sin: 'Math.sin', cos: 'Math.cos', tan: 'Math.tan',
+ asin: 'Math.asin', acos: 'Math.acos', atan: 'Math.atan',
+ sinh: 'Math.sinh', cosh: 'Math.cosh', tanh: 'Math.tanh',
+ asinh: 'Math.asinh', acosh: 'Math.acosh', atanh: 'Math.atanh',
+ pi: 'Math::PI', E: 'Math::E', I: '::Complex::I',
+ dirichlet_eta: 'SymEngine::Utils::evalf_dirichlet_eta',
+ zeta: 'SymEngine::Utils::evalf_zeta', gamma: 'Math.gamma' }.map { |from, to| [/(\b#{from}\b)/, to] }.to_h.freeze
+ def evalf_dirichlet_eta(exp)
+ SymEngine::evalf(SymEngine::dirichlet_eta(exp))
+ end
+ def evalf_zeta(exp)
+ SymEngine::evalf(SymEngine::zeta(exp))
+ end
+ def lambdify_code(exp, syms)
+ str = exp.to_s
@zverok
zverok Jul 25, 2016 Collaborator

It seems this line is unnecessary, you can just do body = REPLACEMENTS.inject(exp.to_s){...} below (and note the variable name change: body seems more meaningful than str, don't you think?..)

@zverok
zverok Jul 25, 2016 Collaborator

Ah, missed the line 60. Then something like:

body = exp.to_s.gsub(/[\d\.]+/, 'Rational(\0,1)')
rubified_body = REPLACEMENTS.inject(body)
@zverok
Collaborator
zverok commented Jul 25, 2016

@isuruf to be honest, current PR's code quality seems pretty reasonable to me. I'm strongly after introducing Rubocop (or other quality check tool) and fix code quality, but maybe in separate PR?..

@rajithv
Contributor
rajithv commented Jul 29, 2016

@isuruf ping

@isuruf isuruf merged commit 67924f4 into symengine:master Jul 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@isuruf
Member
isuruf commented Jul 29, 2016

Sorry, I completely forgot about the PR. Thanks for the work @rajithv, and @zverok for reviewing

@isuruf isuruf referenced this pull request Jul 29, 2016
Closed

Expose lambdify #35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment