-
Notifications
You must be signed in to change notification settings - Fork 5
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
Method v/s manifold #247
Comments
This way, we have the ability to disambiguate methods from manifolds. |
I'm not sure this was what we specifically agreed! :
Two traits is a bit confusing and ambiguous for users, another option is that keyword arguments could be preffered and dispatch to traits could be on There is a point where combinatorial positional traits becomes confusing to read. I'm not sure if two is too many but three definately would be! |
Hmm, we could do arclength(geom; manifold, method)
arclength(manifold, geom; method) and then dispatch is done internally? I'm still not sure I like the underscore architecture though, it seems a bit fishy for extendability... Definitely agree on combinatorial position traits being bad. But dispatching off keyword arguments is probably bad form. I had also considered making algorithms an optional part of this informal "operation interface". But the problem is that you can't shoehorn e.g |
Ok right underscore methods are not so good in an interface. It would have to be something else. So I guess its preferable to have those positional methods as long as we have the keyword methods as the main things in the docs. |
Talking with @TheCedarPrince we came up with what I consider a decent interface: struct SomeManifold{T} <: Manifold
val::T
end
struct SomeAlgorithm{T} <: Algorithm
val::T
# or maybe
# kw1
# kw2
# ...
end
# user space
arclength(geom; kwargs...) = arclength(best_manifold(geom), geom)
# some specification / preference
arclength(m::Manifold, geom; kwargs...) = arclength(m, best_algorithm(m, geom; kwargs...), geom)
arclength(a::Algorithm, geom #= NO KWARGS ALLOWED =#) = arclength(applicable_manifold(a, geom), a, geom)
# user/algorithm developer space
arclength(m::Manifold, alg::Algorithm, geom #= NO KWARGS ALLOWED =#) # main dispatch point and then, in user space, # the maximum the user can do
arclength(Planar(), GEOS(; maybe_kwargs...), geom)
# but also
arclength(Geodesic(), PROJ(; maybe_kwargs...), geom) == arclength(PROJ(), geom) == arclength(Geodesic(), geom)
# generally the user will see
arclength(geom; maybe_kwargs...) # auto choose, collapse kwargs into algorithm
# or
arclength(Planar(), geom; maybe_kwargs...) # auto choose - usually native Julia in this instance, collapse kwargs into algorithm
# and if calling out to a library, we know which manifold it should be in
arclength(GEOS(), geom) |
I don't love having two arguments for traits. I would use one argument for both - then users and other packages can pass around one bundle rather than two. So But either
Top level functions |
It's agreed that we should have
and then
then we, or users, define methods for the API
The text was updated successfully, but these errors were encountered: