Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend parsing of documentation #150

Merged
merged 38 commits into from May 4, 2022
Merged

Extend parsing of documentation #150

merged 38 commits into from May 4, 2022

Conversation

Joel-Dahne
Copy link
Collaborator

This extends which files in the Arb documentation we parse. I have now added parsing of all files except the deprecated fmpr module. Most of the newly parsed files don't actually add any new methods because they use types that we don't implement yet. Nevertheless I think it is nice to parse them anyway so that we are explicit in which types are not implemented. Some new methods are also included, for example acb_dft. This also updates the parsed files to version 2.22 of Arb, which is the one we are on, adding a few new methods.

One thing I want to add in the future is special parsing of the fpwrap module, so that the can be used conveniently. I also want to look over the newly added methods to Arb and see if we should do something with any of them, for example there is now an erfinv function that should be added to the SpecialFunctions interface. But all of this is for a future PR.

@kalmarek
Copy link
Owner

I took my liberty to add preliminary support for fpwrap. I don't think we can actually add support for julia-like usage since most of the functions (e.g. cos(::Float64)) would amount to piracy... Or maybe you have something else in mind?

What we can definitely do is to add default value (0) for int flags carg. Except in fpwrap it is used in ~15 other methods:

./acb_calc.jl:#ni arbcall"int acb_calc_integrate_gl_auto_deg(acb_t res, slong * num_eval, acb_calc_func_t func, void * param, const acb_t a, const acb_t b, const mag_t tol, slong deg_limit, int flags, slong prec)"
./acb_elliptic.jl:arbcall"void acb_elliptic_rf(acb_t res, const acb_t x, const acb_t y, const acb_t z, int flags, slong prec)"
./acb_elliptic.jl:arbcall"void acb_elliptic_rg(acb_t res, const acb_t x, const acb_t y, const acb_t z, int flags, slong prec)"
./acb_elliptic.jl:arbcall"void acb_elliptic_rj(acb_t res, const acb_t x, const acb_t y, const acb_t z, const acb_t p, int flags, slong prec)"
./acb_elliptic.jl:arbcall"void acb_elliptic_rj_carlson(acb_t res, const acb_t x, const acb_t y, const acb_t z, const acb_t p, int flags, slong prec)"
./acb_elliptic.jl:arbcall"void acb_elliptic_rj_integration(acb_t res, const acb_t x, const acb_t y, const acb_t z, const acb_t p, int flags, slong prec)"
./acb_hypgeom.jl:arbcall"void acb_hypgeom_2f1_transform(acb_t res, const acb_t a, const acb_t b, const acb_t c, const acb_t z, int flags, int which, slong prec)"
./acb_hypgeom.jl:arbcall"void acb_hypgeom_2f1(acb_t res, const acb_t a, const acb_t b, const acb_t c, const acb_t z, int flags, slong prec)"
./acb.jl:#ni arbcall"void acb_lambertw(acb_t res, const acb_t z, const fmpz_t k, int flags, slong prec)"
./acb_poly.jl:#ni arbcall"void _acb_poly_lambertw_series(acb_ptr res, acb_srcptr z, slong zlen, const fmpz_t k, int flags, slong len, slong prec)"
./acb_poly.jl:#ni arbcall"void acb_poly_lambertw_series(acb_poly_t res, const acb_poly_t z, const fmpz_t k, int flags, slong len, slong prec)"
./arb_fmpz_poly.jl:#ni arbcall"void arb_fmpz_poly_complex_roots(acb_ptr roots, const fmpz_poly_t poly, int flags, slong prec)"
./arb.jl:arbcall"void arb_lambertw(arb_t res, const arb_t x, int flags, slong prec)"
./arb_poly.jl:arbcall"void _arb_poly_lambertw_series(arb_ptr res, arb_srcptr z, slong zlen, int flags, slong len, slong prec)"
./arb_poly.jl:arbcall"void arb_poly_lambertw_series(arb_poly_t res, const arb_poly_t z, int flags, slong len, slong prec)"

@Joel-Dahne
Copy link
Collaborator Author

I actually had something slightly different in mind! I pushed some WIP code now. My thinking is that the fpwrap part of Arb is sort of its own separate thing and it would make sense to treat it differently than the rest. I added an ArbFPWrapfunction type for generating the code for these functions. The idea is then to generate them into something similar to this

function fpwrap_exp(x::Union{Float16, Float32, Float64}; safe::Bool = true, accurate_parts::Bool = false, correct_rounding::Bool = false, work_limit::Integer = 8)
    res = Ref{Float64}()
    flags::Cint = accurate_parts + correct_rounding * 2 + work_limit * 65536
    success = ccall(Arblib.@libarb("arb_fpwrap_double_exp"), Cint, (Ptr{Float64}, Ref{Float64}, Float64, Int32, Int32), res, res, x, flags, flags)
    if iszero(success) || !safe
        return res[]
    else
        error("unable to evaluate accurately")
    end
end

This keeps the fpwrap prefix, which I think makes sense to distinguish the methods from the rest. It also makes it easy to set the different flags as well as toggle throwing on inaccurate results or not. It still doesn't handle all cases properly. For example it only supports one result and doesn't handle input vectors well, though there are very few such functions in the module.

What do you think about this approach? One idea would also be to have both these wrappers and the more low-level wrappers you implemented.

@kalmarek
Copy link
Owner

@Joel-Dahne this looks good! The compiler seems to eludes the creation of Ref ;) It can't do so in the case of exp! that's generated through my code, so I think your approach is much better!

julia> @btime fpwrap_exp($3.0)
  286.898 ns (0 allocations: 0 bytes)
20.085536923187668

julia> function f(x)
           rs = [one(x)]
           Arblib.exp!(rs, x)
           return rs[]
       end
f (generic function with 1 method)

julia> @btime f(3.0)
  334.448 ns (1 allocation: 64 bytes)
20.085536923187668

@Joel-Dahne
Copy link
Collaborator Author

Even better then! I'll make an attempt at a more serious implementation then. I think it might try to extract the methods related to parsing to a separate submodule, then it wont clutter the Arblib namespace so much.

@kalmarek
Copy link
Owner

I was actually thinking about it as well

@Joel-Dahne
Copy link
Collaborator Author

I got caught up in other things this week, but I'll come back to this when I get some time again! 😃

@kalmarek
Copy link
Owner

kalmarek commented Apr 2, 2022

no rush @Joel-Dahne, I will be pretty preoccupied for the next 3 weeks as I'll be teaching the next semester ;)

Copy link
Owner

@kalmarek kalmarek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides two small nitpicks (that you may as well ignore ;) this is really solid!

Moving argument detection to CArg was overlooked for quite some time ;)
separating ArbCall to its own module we have already discussed.

👍 💯

src/ArbCall/ArbFunction.jl Outdated Show resolved Hide resolved
# The title is on the form
# **filename.h** -- some title
# Take only the part after --
push!(sections, (uppercasefirst(strip(split(title, "--", limit = 2)[2])), []))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably better handled via regex?

m = match(r"\*{2}\w+\.h\*{2}\s-{2}\s(?<title>[\w\s]*)", title)
@assert !isnothing(m[:title])
push!(sections, (uppercasefirst(m[:title]), []))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must say I find the non-regex version easier to read :) I'll see what I do in the end!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's me trying to be too specific :D Is

m = match(r"(.*)-- (?<title>.*)", title)

better?

@Joel-Dahne
Copy link
Collaborator Author

Thanks for the comments! I though I would also add some more tests to get the test coverage up, otherwise it will annoy me forever! Once that is done I'll merge it.

@Joel-Dahne
Copy link
Collaborator Author

I remembered I had one design decision I wanted to check with you. For the arb_fpwrap methods there is a keyword argument that determines if the throws an error on failure to converge or returns NaN. for now I called this argument safe, however I'm not sure if this is the best name. I have been considering return_nan. Do you have any thoughts?

@kalmarek
Copy link
Owner

kalmarek commented Apr 21, 2022

hmm, maybe error_on_failure is more descriptive? I don't have a really good candidate though.

From the user perspective there I think it's best to have a global switch that you can flip to find the first NaN that happens in your computations though...

@kalmarek
Copy link
Owner

kalmarek commented May 2, 2022

@Joel-Dahne this seems to be ready to go? fixing formatting + bumping version is all it needs I think

@Joel-Dahne
Copy link
Collaborator Author

Sorry, I got disrupted finishing up a paper. But we just uploaded it to arxiv so now I will have some time! 😀

I'll switch the name of the flag we discussed, probably taking your name, and will look at implementing a global switch for the default value. Apart from that it should only need an update of the formatter version and a bump of the version and be good to go! I'll probably get it done throughout the week!

Previously the error_on_failure argument for fpwrap methods would
default to false. Now it still defaults to false but this can be
changed using the fpwrap_error_on_fauilure_default_set method. This
allows switching this on or off globally for easier debugging whens
searching for what produces a NaN. The current implementation leads to
zero overhead, it only incurs a compile time cost.

See comments in #138 and
#150 for some context.
@Joel-Dahne
Copy link
Collaborator Author

Okay, now I think this should be good to go! I implemented the toggle for default argument as discussed in #138. You could maybe check if this looks reasonable to you.

The test coverage for ArbCall is almost 100%! The only things missing is one branch meant for if Arb returns an incorrect value and two lines which seems to be bugs, at least they should be included as far as I can tell.

@kalmarek
Copy link
Owner

kalmarek commented May 4, 2022

maybe the only thing that is missing is incrementing the version ;)

@Joel-Dahne Joel-Dahne merged commit 8644974 into master May 4, 2022
@Joel-Dahne Joel-Dahne deleted the extend-parsing branch May 4, 2022 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants