Idiomatic Julia

Question

Idiomatic Julia

o314 opened this issue 3 years ago · 15 comments

Thanks for taking time to bring vips to julia !
Here are a few tips about writing more idiomatic Julia that may help :

# ~ @ https://github.com/jcupitt/julia-vips/blob/1e017ba451a377152e5bf802a2d4c47faff0d2bf/try1.jl#L11-L17
_vips_version(flag) = ccall((:vips_version, _LIB_FP), Int32, (Int32,), flag)
version = join([string(_vips_version(x)) for x=0:2], ".")
println("libvips version = $(version)")

let's remark that :

using Test
@test VERSION isa VersionNumber # julia's one

and so, it can be tuned into

const _LIB_FP = :libvips
# const _LIB_FP = raw"c:\bin\vips-dev-8.11\bin\libvips-42.dll"

const VIPS_VERSION =
    let f(flag) = ccall((:vips_version, _LIB_FP), Int32, (Int32,), flag) ,
        s = [f(i) for i in 0:2] ;
        VersionNumber(s...)
    end
@show VIPS_VERSION

Factorize code repetition via macro

set(gvalue, b::Bool) = ccall((:g_value_set_boolean, _LIB_FP), Cvoid, (Ptr{GValue}, Cint), Ref(gvalue), b)
set(gvalue, i::Int64) = ccall((:g_value_set_int64, _LIB_FP), Cvoid, (Ptr{GValue}, Clonglong), Ref(gvalue), i)
set(gvalue, i::UInt64) = ccall((:g_value_set_uint64, _LIB_FP), Cvoid, (Ptr{GValue}, Culonglong), Ref(gvalue), i)
set(gvalue, f::Float64) = ccall((:g_value_set_double, _LIB_FP), Cvoid, (Ptr{GValue}, Cdouble), Ref(gvalue), f)
set(gvalue, f::Float32) = ccall((:g_value_set_float, _LIB_FP), Cvoid, (Ptr{GValue}, Cfloat), Ref(gvalue), f)
set(gvalue, s::String) = ccall((:vips_value_set_ref_string, _LIB_FP), Cvoid, (Ptr{GValue}, Cstring), Ref(gvalue), s)

can be tuned into (not tested)

for (fun, typ, ctyp) in (
        (:g_value_set_boolean,          Bool,               Cint),
        (:g_value_set_int64,            Int64,              Clonglong),
        (:g_value_set_uint64,           UInt64,             Culonglong),
        (:g_value_set_double,           Float64,            Cdouble),
        (:g_value_set_float,            Float32,            Cfloat),
        (:vips_value_set_ref_string,    String,             Cstring))
    ;
    @eval set(gvalue, a::$typ) = ccall(($fun, _LIB_FP), Cvoid, (Ptr{GValue}, $ctyp), Ref(gvalue), a)
end

contd

gboolean = gtype("gboolean")
gint = gtype("gint")
guint64 = gtype("guint64")
gdouble = gtype("gdouble")
gstr = gtype("gchararray")
genum = gtype("GEnum")
gflags = gtype("GFlags")
gobject = gtype("GObject")
image = gtype("VipsImage")
array_int = gtype("VipsArrayInt")
array_double = gtype("VipsArrayDouble")
array_image = gtype("VipsArrayImage")
refstr = gtype("VipsRefString")
blob = gtype("VipsBlob")
source = gtype("VipsSource")
target = gtype("VipsTarget")

can be tuned into (not tested)

for (nm, typ) in (
        (:gboolean,                     "gboolean")
        (:gint,                         "gint")
        (:guint64,                      "guint64")
        (:gdouble,                      "gdouble")
        (:gstr,                         "gchararray")
        (:genum,                        "GEnum")
        (:gflags,                       "GFlags")
        (:gobject,                      "GObject")
        (:image,                        "VipsImage")
        (:array_int,                    "VipsArrayInt")
        (:array_double,                 "VipsArrayDouble")
        (:array_image,                  "VipsArrayImage")
        (:refstr,                       "VipsRefString")
        (:blob,                         "VipsBlob")
        (:source,                       "VipsSource")
        (:target,                       "VipsTarget"))
    ;
    @eval const $nm = gtype($typ) # const should be uppercased
end

Misc

nothing is an element, Nothing is it's type (dunno if it's useful to state it)

using Test
@test nothing isa Nothing

--
Val{T} can be useful to input lightly typed values into function

# dispatch on type without external typedef
_do(::Val{:jpg}, img) = """i am "$img" and i'am a jpg"""
_do(::Val{:png}, img) = """i am "$img" and i'am a png"""
@test _do(Val(:jpg), "foo.jpg") == """i am "foo.jpg" and i'am a jpg"""
@test _do(Val(:png), "bar.png") == """i am "bar.png" and i'am a png"""

--
NamedTuple can be useful to return lightly typed values from function

# return pseudo type without external typedef
_do2(img; lossy=true) = lossy ? (; fmt=:jpg, img) : (; fmt=:png, img)
@test _do2([0,1,0,1]) == (; fmt=:jpg, img=[0,1,0,1])
@test _do2([0,1,0,1]; lossy=false) == (; fmt=:png, img=[0,1,0,1])

A lot of code seems already quite good otherwise !

Answer 1 · 2022-03-19T10:46:35.000Z

Hi @o314,

Thank you for taking the time to look though my beginner's code, this is very helpful!

I'll try to work though your list of tips this weekend and see if I can understand them.

Answer 2 · 2022-03-19T12:48:46.000Z

I have some questions (of course):

    const _LIB_FP = :libvips

I like the const, but why the _FP suffix?

        (:vips_value_set_ref_string,    String,             Cstring))
    ;
    @eval set(gvalue, a::$typ) = ccall(($fun, _LIB_FP), Cvoid, (Ptr{GValue}, $ctyp), Ref(gvalue), a)

Could you explain why you have a ; here? It seems to work without this (as far as I can see).

Also, I've been wondering how to handle introspection. The ruby bindings for example intercept method_missing and use that to call into libvips, so eg.

image = image.embed(10, 10, 100, 100)

method_missing will catch the call to embed (which the binding does not define), search the libvips binary, find the argument types, convert the 4 ints (if it can), call the libvips operation, and return the result.

Julia doesn't have a mechanism like this, but I wonder if macros would work? During vips startup it could walk the library and use @eval to add a set of functions for the library API. Does that sound reasonable? I suppose it's likely to make startup rather slow.

Also, how would a Julia expert expect a libvips API to look? Something like this, maybe:

image = Vips.Image.new_from_file("k2.jpg")
image = Vips.Image.embed(image, 10, 10, 100, 100)
Vips.Image.save_to_file("k2a.jpg")

It seems rather wordy, but I suppose people can import the libvips namespace.

Answer 3 · 2022-03-19T14:37:36.000Z

... last post, I think I've incorporated your comments, thanks again!

I've got stuck on your set() example though. This code:

https://github.com/jcupitt/julia-vips/blob/main/vips.jl#L64-L78

Fails with:

ERROR: LoadError: UndefVarError: g_value_set_boolean not defined

It looks like after macro expansion it's interpreting this:

                ($fun, _LIB_FP),

As:

                (g_value_set_boolean, _LIB_FP),

And then obviously discovering that the variable does not exist. I tried:

                ("$fun", _LIB_FP), 
                ("$(fun)", _LIB_FP), 
                (:$fun, _LIB_FP), 
                (:($fun), _LIB_FP),

But none of them worked. What do I need to do?

Answer 4 · 2022-03-21T06:36:49.000Z

SIMPLE THINGS FIRST

const _LIB_FP = :libvips
I like the const, but why the _FP suffix?

FP stands for filepath. My own style. Not prescriptive at all there.
Just here to handle what happens according to the fact the lib is in PATH or not...

--

Could you explain why you have a ; here? It seems to work without this (as far as I can see).

My own style. Not prescriptive at all there.
I typically use that to separate two big adjacent blocks of array/tuple.
It improves readability of inlined big data around for, let etc.
It absolutely works without it.

Answer 5 · 2022-03-21T06:39:10.000Z

A BIT HARDER - INSTRUMENTATION

Also, I've been wondering how to handle introspection. The ruby bindings for example intercept method_missing and
use that to call into libvips, so eg.
image = image.embed(10, 10, 100, 100)
method_missing will catch the call to embed (which the binding does not define),
search the libvips binary, find the argument types, convert the 4 ints (if it can), call the libvips operation, and return the result.

Julia doesn't have a mechanism like this, but I wonder if macros would work?
During vips startup it could walk the library and use @eval to add a set of functions for the library API.
Does that sound reasonable? I suppose it's likely to make startup rather slow.

Act proactively. no intercept with introspection but rather walk the library and use @eval.
This will slows down things. Precompilation will help to catch up with some Base.precompile calls.
The julia cli has an hidden flag (see julia --help-hidden) eg. trace-compile that will output a lot of useful
function refs that need to be precompiled to increase perf.

julia --trace-compile=_precompile.jl vips.jl

See also

issues around module initialization and precompilation
concerning __init__ etc
https://github.com/JuliaPackaging/Yggdrasil/blob/master/I/ImageMagick/build_tarballs.jl to go further

Answer 6 · 2022-03-21T06:42:22.000Z

A BIT HARDER - IO API

Also, how would a Julia expert expect a libvips API to look? Something like this, maybe:

image = Vips.Image.new_from_file("k2.jpg")
image = Vips.Image.embed(image, 10, 10, 100, 100)
Vips.Image.save_to_file("k2a.jpg")
It seems rather wordy, but I suppose people can import the libvips namespace.

There may be two levels of commitment there :

The simple one as in MD, Json that may require

load(io::IO)::Image = error("not implemented")
loadfile(filename::AbstractString)::Image = error("not implemented")
savefile(img, filename::AbstractString) = error("not implemented")
Base.getindex(a::Vips::Image, a::AbstractRange{Int}, a::AbstractRange{Int}) = error("not implemented")

Example

import Vips
img = Vips.loadfile("k2.jpg")
img = Vips.loadfile("k2.jpg", 10:110, 10:110)
img2 = img[10:110, 10:110]
Vips.savefile(img, "k2a.jpg")

See

The second one, requiring more engagement, like in ImageMagick.jl, FileIO.jl
will use generic load/save plugged into a federated registry built upon mime type

I dunno believe there is a clear consensus around image io, and particulary subimage addressing today.
It exists the cluster juliaimages that deals with it https://juliaimages.org/stable/tutorials/conversions_views/
IMHO in an over-complicated way

JUST TO REMIND

A = [10i+j for i in 1:9, j in 1:9]
B = A[3:8, 3:5]
using Test
@test B == [
    33  34  35;
    43  44  45;
    53  54  55;
    63  64  65
    73  74  75;
    83  84  85]

# @code_lowered (()->(B = A[3:8, 3:5]))()
# => will dispatch to
Base.getindex(a::Vips::Image, a::AbstractRange{Int}, a::AbstractRange{Int}) = error("not implemented")

Answer 7 · 2022-03-21T06:52:41.000Z

HARDER ONE NOW

You turn to macro and esc. There are issues around it clearly. However it's a very common practice in Julia to use both @eval loop and macro. At this stage of complexity, @eval loop should/must be enough. Requiring too much esc in macro is a code smell IMO.

I think you should try to use only one level of module eg. Vips, and flatten everything inside it. That scales perfectly to hundreds LOC, and that will help to simplify issues around esc. My time is running out, i will try to give better perk asap/as soon as I can

SEE

meanwhile https://github.com/JuliaIO/FileIO.jl/blob/master/src/loadsave.jl

Answer 8 · 2022-03-21T09:16:18.000Z

Thanks @o314, I'll work though these.

Answer 9 · 2022-03-21T14:34:29.000Z

I've tried to incorporate your comments, thanks again!

I still can't define functions in a loop. This works:

macro define_set(fun, typ, ctyp)
    quote
        function $(esc(:set))($(esc(:gvalue)), $(esc(:a))::$(esc(typ)))
            ccall(
                ($fun, _LIBNAME),   
                Cvoid,
                (Ptr{GValue}, $ctyp),
                Ref($(esc(:gvalue))),
                a)
        end
    end
end

@define_set(:g_value_set_boolean,       Bool,    Cint)
@define_set(:g_value_set_int64,         Int64,   Clonglong)
@define_set(:g_value_set_uint64,        UInt64,  Culonglong)
@define_set(:g_value_set_double,        Float64, Cdouble)
@define_set(:g_value_set_float,         Float32, Cfloat)
@define_set(:vips_value_set_ref_string, String,  Cstring)

But it's rather ugly.

If you add a loop, all the quoting breaks and I can't find a fix. I'll try to read the macro chapter again.

Answer 10 · 2022-03-21T15:09:51.000Z

By poking aroung something like

using Test
for a in (:foo,:bar)
    b = @eval $(QuoteNode(a))
    @test b isa Symbol
    @show b
    println("ccall(($(repr(b)), :mylib), (Cvoid), (Cint,), -1)")
end

You should be able to get a correct MWE with @eval loop

Answer 11 · 2022-03-26T12:58:31.000Z

By a lot of trial and error I finally found the solution. You must write:

for (fun, typ, ctyp) in (
    ("g_value_set_boolean",       Bool,    Cint),
    ("g_value_set_int64",         Int64,   Clonglong),
    ("g_value_set_uint64",        UInt64,  Culonglong),
    ("g_value_set_double",        Float64, Cdouble),
    ("g_value_set_float",         Float32, Cfloat),
    ("vips_value_set_ref_string", String,  Cstring))

    @eval begin
        function set(gvalue, a::$typ)
            ccall(($fun, _LIBNAME), Cvoid, (Ptr{GValue}, $ctyp), Ref(gvalue), a)
        end
    end
end

ie. use "strings" not symbols for the library function names. Symbols are automatically turned into variable references by @eval (I think).

Finally some progress! I'll have a go at calling a libvips operation next.

Answer 12 · 2022-03-28T15:07:43.000Z

It's seems to work to ccall with string. Fine.

If you are straight on typing,

for (fun, gtyp, ctyp) in (
    ("g_value_get_boolean", :GBOOLEAN, Cint),
    ("g_value_get_int",     :GINT,     Cint),
    ("g_value_get_int64",   :GINT64,   Clonglong),
    ("g_value_get_uint64",  :GUINT64,  Culonglong),
    ("g_value_get_double",  :GDOUBLE,  Cdouble),
    ("g_value_get_object",  :GOBJECT,  Ptr{GObject}),
    ("g_value_get_float",   :GFLOAT,   Cfloat))

    @eval get_type(gvalue, ::Val{$gtyp}) = ccall(($fun, _LIBNAME), 
        $ctyp, (Ptr{GValue},), 
        Ref(gvalue))
end

can be rewritten with Symbol, as originally, like this :

for (cfun, gtyp, ctyp) in (
    (:g_value_get_boolean, :GBOOLEAN, Cint),
    (:g_value_get_int,     :GINT,     Cint),
    (:g_value_get_int64,   :GINT64,   Clonglong),
    (:g_value_get_uint64,  :GUINT64,  Culonglong),
    (:g_value_get_double,  :GDOUBLE,  Cdouble),
    (:g_value_get_object,  :GOBJECT,  Ptr{GObject}),
    (:g_value_get_float,   :GFLOAT,   Cfloat))

    @eval get_type(gvalue, ::Val{$gtyp}) = ccall(($(QuoteNode(cfun)), _LIBNAME), 
        $ctyp, (Ptr{GValue},), 
        Ref(gvalue))
end

compare with
methods(get_type) |> collect |> first |> Base.uncompressed_ast

Answer 13 · 2022-03-28T15:08:32.000Z

function to_enum(gtype, value)
    if value <: String
        value = enum_from_nick(gtype, value)
    end
    value
end

may be rewritten like this

to_enum(gtype, ::Type{T}) where {T<:AbstractString} = enum_from_nick(gtype, T)
to_enum(gtype, ::Type{T}) where {T} = T

see https://docs.julialang.org/en/v1/manual/performance-tips/#Break-functions-into-multiple-definitions

Answer 14 · 2022-03-29T20:37:22.000Z

Of course! Silly me, I keep forgetting to use multiple dispatch.

I suppose that could also be:

to_enum(gtype, value::AbstractString) = enum_from_nick(gtype, value)
to_enum(gtype, value) = value

Which is maybe simpler.

Answer 15 · 2022-03-30T20:56:25.000Z

Those both forms can work :

f(x, ::Type{T}) where {T<:Integer} = "f for int from $x"
f(x, ::Type{T}) where {T} = "f for any from $x"


g(x, y::Integer) = "f for $y from $x"
g(x, y) = "f for $y from $x"

But they will be called differently

# first form will not carry any integer value, it just knows you're dealing with an int
using Test
@test f(2.0, Int) == "f for int from 2.0"
@test f(2, Char) == "f for any from 2"

# second form will carry int value
@test g(2.0, 1) == "f for 1 from 2.0"
@test g(2, 'y') == "f for y from 2"

In the context of mapping ccall and c struct to their julia equivalents
I am not sure which one will be the most appropriated according to the precise code we consider.
it may depend !