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

Functor overloading for custom types? #65

Open
lassepe opened this issue Jul 17, 2020 · 12 comments
Open

Functor overloading for custom types? #65

lassepe opened this issue Jul 17, 2020 · 12 comments

Comments

@lassepe
Copy link

lassepe commented Jul 17, 2020

I am trying to figure out what is the correct way to overload the transformation functors for custom types? For example, if I want to be able to transform something like GeometryBasics.Line then I can not simply implement:

function (tform::CoordinateTransformations.Transformation)(l::Line)
    Line((tform.(l))...)
end

since the call to the functor then becomes ambiguous (There are methods with a more specific Transformation subtype but a less specific argument type (input to the transformation)). So instead I would overload the functor for concrete subtypes of Transformation, e.g.:

function (tform::CoordinateTransformations.AffineMap)(l::Line)
    Line((tform.(l))...)
end

function (tform::CoordinateTransformations.LinearMap)(l::Line)
    Line((tform.(l))...)
end

But this does not seem very clean since it leads to a lot of code duplication.
Is there some kind of best practice for this? Should I not be overloading these functors at all (since it may be considered type piracy) but instead define my own thin wrapper like apply_transformation(tform, x)?

It may be good to have some brief documentation on this somewhere in the README.

@c42f
Copy link
Member

c42f commented Jul 21, 2020

Should I not be overloading these functors at all (since it may be considered type piracy) but instead define my own thin wrapper like apply_transformation(tform, x)?

Having a wrapper seems rather annoying! I think it's ok to do some type piracy in the short term, especially at the application level (not so much in a reusable library).

Perhaps part of the solution would be for CoordinateTransformations itself to be more specific about which types which transformations may take. For example, maybe we should be restricting ourselves to x::AbstractVector? @andyferris do you think that would work out? I can't remember why we allowed x::Any in transformations like LinearMap; it seems a bit hopeful.

Generally I think transforming extended non-point like objects is a bit tricky: if they're based on a discretization like Line, you can transform the endpoints but there's no guarantee that this makes any sense for general Transformations into curved coordinate systems. It might be a good approximation if the Line is small compared to the local curvature, but it's equally possible to get a terrible approximation.

@lassepe
Copy link
Author

lassepe commented Jul 25, 2020

Generally I think transforming extended non-point like objects is a bit tricky: if they're based on a discretization like Line, you can transform the endpoints but there's no guarantee that this makes any sense for general Transformations into curved coordinate systems.

Okay, this makes a lot of sense. In view of this fact it seems sensible to overload the functors for the concrete subtypes of Transformation.

@c42f
Copy link
Member

c42f commented Jul 28, 2020

overload the functors for the concrete subtypes of Transformation.

I think for piecewise linear geometry you want to overload for AbstractAffineMap in principle. But then you hit the original problem that we're too loose with our types in the package. So we probably need to fix this.

Many transformations are almost-affine at practical scales and it often makes sense to ignore the curvature and do a pointwise transformation of the vertices for piecewise linear geometry. But I'm not sure what's the best way is to model this — the validity depends on the scale of the geometry, the curvature of the coordinate system, and the tolerance in your actual application. A practical option might be to have a ApproximatelyAffineWrapper <: AbstractAffineMap which can wrap any Transformation. If the user uses this, it would be an assertion that it is in fact affine-enough (even though not exact). Then have all piecewise linear geometry interact with AbstractAffineMap.

There's other things you could consider, such as subdividing piecewise geometry according to some tolerance. But whether this is a good idea is very application-specific (in particular, it makes the transformation very non-invertible).

Related note - you can create a locally affine approximation of any transformation using one of the AffineMap constructors. But depending on your application this might not be as good as doing a pointwise vertex transformation in practice, as the pointwise transformation can be fully invertible (from the point of view of the vertices, if not the edges).

@andyferris
Copy link
Contributor

andyferris commented Jul 28, 2020

Can you put methods on (instances of) abstract types now? I thought that was a no-go?

Generally yes we should probably only support AbstractVector out of the box for LinearMap and AffineMap, I feel.

While I think it's fine to transform a complex object like a line through a transform meant for points, these days I also often think of complex geometric objects as containing sets of points and might be modelled to behave as collections - in which case map(transform, complex_object) or transform.(complex_object) makes perfect sense there, and can be overloaded to your heart's content.

@lassepe
Copy link
Author

lassepe commented Jul 28, 2020

Can you put methods on (instances of) abstract types now? I thought that was a no-go?

You can do this as of JuliaLang/julia#31916

@andyferris
Copy link
Contributor

Oh right - I had forgotten!

Well, adding call overloads to Transforms was definitely a wanted feature since in the first version of this package.

I’d say tighten our built-in definitions to AbstractVector{<:Number} and we should let users overload generic transforms for extended objects.

@c42f
Copy link
Member

c42f commented Jul 29, 2020

these days I also often think of complex geometric objects as containing sets of points and might be modelled to behave as collections

Yes I like this model.

  • in which case map(transform, complex_object) or transform.(complex_object) makes perfect sense there

This seems ok, but I don't think it addresses the core difficulty which is that mapping such sets through non-affine transformations is generally approximate, and that various approximations might be applicable depending on application. So you could choose one particular approximation but somebody else could end up needing a different one. Whether it's called map(transform, complex_object) or transform(complex_object), the problem still remains.

@andyferris
Copy link
Contributor

andyferris commented Jul 30, 2020

My favourite way to deal with such approximations is to avoid them by making such transforms lazy, and you can still determine predicates like point in transform(object) by dispatching that to inv(transform)(point) in object. In most cases you could probably find a bounding box that is relatively tight yet valid. You should be able to generate approximate wire meshes for display. Etc. These few operations alone solve a decent fraction of my previous usage of geometry.

But yeah, it totally depends on application! Not sure what that means for a package like this?

@c42f
Copy link
Member

c42f commented Jul 30, 2020

by dispatching that to inv(transform)(point) in object

Yes! I almost wrote that you could have a lazy version :-) But again, it does depend on application a lot - the lazy version using the inverse would work for pointwise containment queries, but not for many interesting things such as geometric intersection when the two objects are not in the same source coordinates. Also it's likely to be less efficient.

it totally depends on application! Not sure what that means for a package like this?

As a fairly simple and arguably "core" geometric package, I think it means we leave these operations undefined when the best method is ambiguous and application-specific. Or at most, provide some of the simpler versions in an explicitly opt-in way. (For example, the idea of the ApproximatelyAffineMap wrapper I mentioned above could be one way to let people opt into a particular approximation.)

@BrianGun
Copy link

BrianGun commented May 17, 2022

Has any action been taken on this issue? I want to non-uniformly scale a tesselation of a sphere. Coming from a graphics background I naively expected that I could combine CoordinateTransformations and GeometryBasics to transform meshes. But applying a transformation to a geometry object or a Tessellation object causes a run time error. What is the recommended solution? In graphics we use trees of these transformations to create and animate scenes with each leaf holding a geometric object and the interior nodes holding transformations. Is it not possible to combine CoordinateTransformations and GeometryBasics this way? If not, what's the recommended alternative?

@andyferris
Copy link
Contributor

@BrianGun someone would need to provide overloads to support transforming those geometries. Triangular meshes would be pretty easy to implement since you’d just need to map the transform over the array of vertices. Points, multi points, lines and multi lines are similarly ok.

There’s a slight difficulty in that non-Affine transformations don’t preserve straight lines and flat planes and the output is only an approximation (and sometimes a really poor one). Eg a polygon is no longer a polygon (with the exception of triangles) which violates an assumption of the widespread “simple geometry types”. Long geodesic lines in a geometric projection across the surface of earth become lines going through the centre of Earth (transforming a line into a multi line, arc or multi-arc might be more appropriate). Etc. Providing such transformations by default runs the risk of being a foot gun for someone, somewhere, someday (and more likely for geodesists - everyone, everywhere, all the time).

@andyferris
Copy link
Contributor

Triangular meshes would be pretty easy to implement

To follow up @BrianGun - for your use case in particular it's probably as simple as map!(transform, sphere.vertices) for an in-place transformation.

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

4 participants