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

Method v/s manifold #247

Open
asinghvi17 opened this issue Jan 10, 2025 · 7 comments
Open

Method v/s manifold #247

asinghvi17 opened this issue Jan 10, 2025 · 7 comments

Comments

@asinghvi17
Copy link
Member

asinghvi17 commented Jan 10, 2025

It's agreed that we should have

arclength(geom; manifold = discover_manifold(geom), method = nothing) = arclength(method, manifold, geom)

and then

arclength(manifold, geom) = arclength(nothing, manifold, geom)
arclength(::Nothing, manifold, geom) = arclength(BEST_METHOD_FOR_MANIFOLD, manifold, geom)

then we, or users, define methods for the API

@asinghvi17
Copy link
Member Author

This way, we have the ability to disambiguate methods from manifolds.

@rafaqz
Copy link
Member

rafaqz commented Jan 12, 2025

I'm not sure this was what we specifically agreed! :

arclength(method, manifold, geom)

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 _arclength, or just one trait would be allowed as the first argument.

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!

@asinghvi17
Copy link
Member Author

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 GEOS() into that framework, so there has to be some form of space for algorithms.

@rafaqz
Copy link
Member

rafaqz commented Jan 12, 2025

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.

@asinghvi17
Copy link
Member Author

asinghvi17 commented Feb 8, 2025

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)

@asinghvi17
Copy link
Member Author

@evetion @rafaqz you were both there at the initial discussion, what do you guys think?

@rafaqz
Copy link
Member

rafaqz commented Feb 8, 2025

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 arclength(GEOS(Geodesic(; kw...); kw...), x) would be the full specification syntax.

But either arclength(GEOS(), x) or arclength(Geodesic(), x) would work as well, but needing clarification with applicable_manifold or best_algorithm to end up with the arclength(Algorithm(Manifold()), x) call.

applicable_manifold could then accept the function for clarifying dispach applicable_manifold(::typeof(arclength), ::Geos) = Geodesic()

Top level functions func(trait, x) could just call _func(resolve_alg_and_manifold(func, trait), x) or something.

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

No branches or pull requests

2 participants