pjssilva / Algencan.jl

JuMP / MathProgBase interface for Algencan
Other
7 stars 4 forks source link

Getting rid of global variables #16

Closed odow closed 5 years ago

odow commented 5 years ago

Best practice is to avoid things like https://github.com/pjssilva/Algencan.jl/blob/dadca037b13b6f0ebba7c7d9202d236968f04999/src/Algencan.jl#L534

Basically instead of keeping a global around:

global_model = nothing

function foo()
    model::Model = global_model
    solve(model)
end

function bar()
    global global_model = Model()
    foo()
end

Re-define foo to take model as an argument, and then create an anonymous function around foo to trap the instance of model in a closure.

function foo(model)
    solve(model)
end

function bar()
    model = Model()
    foo_with_model = () -> foo(model)
    foo_with_model()
end

p.s. you should also make MPB a const https://github.com/pjssilva/Algencan.jl/blob/dadca037b13b6f0ebba7c7d9202d236968f04999/src/Algencan.jl#L38

chkwon commented 5 years ago

@odow Did you mean

    foo_with_model = () -> foo(global_model)

I was wondering why you need an anonymous function here. Can't you just do foo(global_model)?

odow commented 5 years ago

Oops, yes. I've updated the post. In my example, yes. But it makes sense in the context of the usage in the code I linked to.

pjssilva commented 5 years ago

I got the idea. I am already working on this. I am doing some final tests and, hopefully, a fix should be released soon. It needed an extra "trick", though. The closure will be used as a parameter for the macro @cfunction in Algencan.jl code. Since the closure is created in running time this first argument need to be prepended by a $ (a dollar sign) to tell the macro to create the C function pointer in running time. Once I realized that, it looks that I got it working.

odow commented 5 years ago

You might want to benchmark the running time before merging just in case, but style wise, this is more "Julian."

pjssilva commented 5 years ago

That is exactly what I doing right now. By one side, there will be some compilation at run time, which might slow things down a bit for very small problems. On the other side, Julia might get better optimizations without the global. Let us see.

pjssilva commented 5 years ago

The benchmark finished (there is always a couple of problems that take forever just to fail in the end..). The version with closures might be a little faster, the difference is not important. I just committed it and it is time to close this issue. I will make a release with this new version as I solved some issues with the build system.