Recipe for defining custom manifolds #37

marcrasi · 2020-04-29T05:48:25Z

This PR adds a recipe in doc/DifferentiableManifoldRecipe.md that describes how to define custom manifolds. To follow the recipe, all you need to do is define a global coordinate system, define a local coordinate type, and define exp and log maps that convert between them. Everything else is automatically derived.

I hope this will make it easier to define Rot3 and Pose3.

This PR also:

Adds supporting code in Manifold.swift.
Rewrites Rot2 using the recipe, to demonstrate how it works.

doc/DifferentiableManifoldRecipe.md

ProfFan · 2020-04-29T15:38:46Z

This looks amazing! I will give this a shot and see if it works for Pose2, and then a review.

ProfFan · 2020-04-30T16:28:23Z

@marcrasi I tried it on Pose2 and it works great! There is a small error though, the global update should be on the right, so it's self * Rot2Coordinate(local.x) not the other way around.

BTW, can you share your repo so I can push to it with the Pose2 impl?

marcrasi · 2020-04-30T17:29:41Z

Ok, I think I gave you access to my repo!

doc/DifferentiableManifoldRecipe.md

marcrasi · 2020-04-30T22:06:01Z

Migrated Pose2

This commit doesn't take advantage of all the possible simplifications. It should be possible to completely remove the custom derivatives for Pose2.init, Pose2.rot, Pose2.t, as long as you define the correct global (aka retract) and local (aka localCoordinates or inverseRetract) methods.

dellaert · 2020-04-30T22:12:19Z

I think retract and localCoordinates are still the most descriptive and they match the book (at least for retract) and GTSAM. I was trying on local for size as it is short and sweet, matching retract, but it really does not capture the coordinates aspect - and users new to this need all the help they can get.

ProfFan · 2020-05-01T01:05:02Z

@marcrasi Sorry to be late for the party here, but can you do a small review on it? So I can see what did I do wrong :)

dellaert · 2020-05-01T03:34:25Z

So, the experience of trying to implement Rot3 raised a lot of questions for me. The main one: why do we have the CoordinateStorage indirection? This feels quite heavy to me: I want to just implement Rot3 as having a 3x3 matrix R. Why do I need to create another type called Matrix3Coordinate? It seems a lot of what happened in Rot3 was passing through stuff to that type: why not have just 1 type?

Now, I suspect it might have to do something with your comment on Pose2 above: maybe the pass-through is all automatic. and @ProfFan and I just didn't get it yet :-) I guess there might be two things going on here at the same time, and I don't yet have the Swiftness of disambiguating them: (a) protocols/module signatures, (b) generation of automatic functionality.

dellaert · 2020-05-01T03:38:12Z

Another comment, which could impact the discussion. One of the things that is regrettably complex in GTSAM is exactly Rot3: there are two code paths: one that stores a Matrix3, and one that stores a Quaternion. You have to select this via a compile flag. Very clumsy.

In fact, there is a mathematical story that is not entirely captured: both 3x3 matrices and quaternions are valid representations for SO(3) (although, one could argue quaternions are not, really). So, there is group SO(3) which is coordinate free, and you have several different representations to indicate an element of that group. You could argue that one should be able to use both interchangeably.

However - as my previous comment indicates: KISS! I'd lean towards trying to eliminate extraneous types (like the storage thingy) in favor of as little code overhead as possible to create your own manifold and/or Lie group.

marcrasi

@ProfFan, I added a review of the Pose2 with all the simplifications that should be possible.

Sources/SwiftFusion/Geometry/Pose2.swift

marcrasi · 2020-05-01T05:18:45Z

why do we have the CoordinateStorage indirection?

The reason for this indirection is that it reduces the number of "primitive functions" that we need to define derivatives for, which makes it easier to explain in the recipe, and also easier for users to implement.

(By "primitive functions", I mean the functions that are used to access fields of the manifold type and that are used to create new instances of the manifold type. Since every function on manifolds is built out of "primitive functions", specifying the derivatives of the "primitive functions" determines the automatically-derived derivatives of all functions on the manifold.)

When there is a single var coordinate field, that is the only field that we need to specify a derivative for. It's easier to explain how to specify the derivative for var coordinate than to explain how to specify derivatives for all the fields in the manifold.

The alternative of defining custom derivatives for each field in the manifold is also possible -- in fact, it's what Pose2 does before this PR.

So there's currently a tradeoff:

Manifold type with var coordinate. It is easier to specify the derivatives, but everything else is more annoying because you have to go through the indirection.
Manifold type that directly contains all its fields. It is harder to specify the derivatives.

I think there's no way around this tradeoff with the current Swift AD APIs. However, we could add a Swift AD feature that gives you the best of both worlds: a way that you can create a Differentiable type with arbitrary fields, and jointly specify the derivatives of all the fields in a single custom derivative function.

marcrasi · 2020-05-01T05:29:53Z

This discussion makes me realize that this PR does a few distinct things:

The var coordinate indirection that makes it easier to specify the desired derivatives.
The ManifoldCoordinate.global / ManifoldCoordinate.local methods from which we attempt to automatically derive derivatives that correspond with local coordinates.
The "recipe" that documents how to use these things.

My current feelings about these are:

It's a tradeoff, as discussed in my previous comment. In the medium term, we should add the Swift AD feature that makes the tradeoff unnecessary. In the immediate term, I don't have any strong opinion about whether we use it or not.
It's a bad idea to automatically derive the derivatives from these. This is because of the inefficiency and the bad behavior when w == 0: Recipe for defining custom manifolds #37 (comment) . I should change it so that users specify the derivatives directly.
Having a recipe is good. It should be updated to correspond with the final decisions on (1) and (2).

ProfFan · 2020-05-01T16:52:29Z

I updated the implementation to incorporate @marcrasi 's suggestions and adapted to GTSAM conventions :)

It's a tradeoff, as discussed in my previous comment. In the medium term, we should add the Swift AD feature that makes the tradeoff unnecessary. In the immediate term, I don't have any strong opinion about whether we use it or not.

Agreed!

It's a bad idea to automatically derive the derivatives from these. This is because of the inefficiency and the bad behavior when w == 0: Recipe for defining custom manifolds #37 (comment) . I should change it so that users specify the derivatives directly.
Having a recipe is good. It should be updated to correspond with the final decisions on (1) and (2).

I think it should be possible also to automatically detect numerical stability issues in automatic differentiation? Anyway I think the current Swift AD can handle the control flow in abs(w)<eps, as the update seemed to have no issues.

marcrasi · 2020-05-01T16:55:57Z

Cool, the updates to Pose2 look great now! That simple near-zero approximation is nice, and makes me feel a bit better about (2) from my previous comment.

I think it should be possible also to automatically detect numerical stability issues in automatic differentiation?

Very interesting feature idea. We don't do that now.

Maybe it would even be possible for AD to automatically derive approximations when there is a stability issue.

This is probably a very long term thing that we shouldn't spend too much effort on now.

Anyway I think the current Swift AD can handle the control flow in abs(w)<eps, as the update seemed to have no issues.

Yep, most control flow is handled quite stably now.

dan-zheng · 2020-05-02T01:35:40Z

I think it should be possible also to automatically detect numerical stability issues in automatic differentiation?

There is research on programming languages and numerical stability: "Automatically Improving Accuracy for Floating Point Expressions", Distinguished Paper PLDI 2015

I haven't thought much about incorporating numerical stability analysis or rewriting into Swift. A simple "finite differences" derivative testing function (early prototype) may be able to catch unstable derivatives for continuous functions.

ProfFan · 2020-05-02T03:30:20Z

So, I adapted @dellaert 's PR and added some tests :) However I still get one ICE from Rot3.swift, line 15. I commented that out and everything works beautifully.

ProfFan · 2020-05-02T03:32:05Z

I think the mathematical cause is that we should not differentiate Rot matrix members as they are constrained internally, but I have no idea why it fails in code... @marcrasi any ideas?

ProfFan · 2020-05-02T03:33:16Z

@dan-zheng Yeah I am very interested in that part as well... However I think my experience on the Swift compiler probably would not allow me to touch that yet...

ProfFan · 2020-05-02T05:01:34Z

@marcrasi I pushed in the Rot3 with proper invariant checks :) Will tackle the Pose3 tomorrow.

ProfFan · 2020-05-02T05:51:37Z

@marcrasi I think I found another issue with this: in Pose3 I need to initialize a Rot3 with the Expmap, and the most intuitive way to do it would be:

extension Rot3 {
  @differentiable
  public static func fromTangent(_ vector: Vector3) -> Self {
    var actual = Rot3()
    actual.move(along: vector)
    return actual
  }
}

However this triggers a compiler ICE that says:

Assertion failed: (tanFieldLookup.size() == 1), function getAdjointProjection, file /Users/swiftninjas/s4tf/swift/lib/SILOptimizer/Utils/Differentiation/PullbackEmitter.cpp, line 335.

1.	Swift version 5.2-dev (LLVM b3057cffb6, Swift da7410955d)
2.	While running pass #855 SILModuleTransform "Differentiation".
3.	While canonicalizing `differentiable_function` SIL node   %16 = differentiable_function [parameters 0 1] %15 : $@convention(method) (Vector3, @inout Rot3) -> () // users: %18, %17
4.	While ...in SIL function "@AD__$s11SwiftFusion4Rot3V11fromTangentyAcA7Vector3VFZ__vjp_src_0_wrt_0".
 for 'fromTangent(_:)' (at /Users/proffan/Projects/Development/CV/SLAM/SwiftFusion/SwiftFusion/Sources/SwiftFusion/Geometry/Rot3.swift:33:10)
5.	While processing // differentiability witness for Rot3.move(along:)
sil_differentiability_witness private [parameters 0 1] [results 0] @$s11SwiftFusion4Rot3V4move5alongyAA7Vector3V_tF : $@convention(method) (Vector3, @inout Rot3) -> () {
}

 on SIL function "@$s11SwiftFusion4Rot3V4move5alongyAA7Vector3V_tF".
 for 'move(along:)' (at /Users/proffan/Projects/Development/CV/SLAM/SwiftFusion/SwiftFusion/Sources/SwiftFusion/Geometry/Rot3.swift:18:19)
6.	While generating VJP for SIL function "@$s11SwiftFusion4Rot3V4move5alongyAA7Vector3V_tF".
 for 'move(along:)' (at /Users/proffan/Projects/Development/CV/SLAM/SwiftFusion/SwiftFusion/Sources/SwiftFusion/Geometry/Rot3.swift:18:19)
7.	While generating pullback for SIL function "@$s11SwiftFusion4Rot3V4move5alongyAA7Vector3V_tF".
 for 'move(along:)' (at /Users/proffan/Projects/Development/CV/SLAM/SwiftFusion/SwiftFusion/Sources/SwiftFusion/Geometry/Rot3.swift:18:19)

I think this is somewhat related to higher-order differentiation, is it?

dan-zheng · 2020-05-02T06:01:47Z

However this triggers a compiler ICE that says:

The meaningful crash message is actually just above the stack trace that you shared: it should be an assertion failure or something similar. Could you please edit your comment to include it?

Sorry about the compiler crashers! Some of them are known issues, we can prioritize fixing them.

ProfFan · 2020-05-02T06:05:07Z

@dan-zheng Sorry for that haha 🤣 it's 2am now and I am a bit dizzy...

I updated it :)

ProfFan · 2020-05-02T06:06:29Z

@dan-zheng Also pushed the source here, so you can take a look if needed :)

marcrasi · 2020-05-02T06:29:07Z

@ProfFan, I pushed a fix for that latest error. It's the same thing, just getting triggered in a different way.

Both of these are caused by accessing coordinateStorace, which is (intentionally) not differentiable. (The move(along:) method accesses coordinateStorage). Hopefully this will become clearer when the compiler gives you a proper error message about not being able to access coordinateStorage in a differentiable function :).

it's 2am now and I am a bit dizzy...

Take care, don't overwork :)!

ProfFan · 2020-05-02T16:20:58Z

Pushed Pose3 with tests from GTSAM.

@marcrasi @dellaert Please (p)review :)

dellaert

I have not done a full review yet as I'm stuck in understanding the protocol. So, this is not a review as such but rather a couple of questions that will allow me to understand. Some of the answers should probably make it to the comments.

dellaert · 2020-05-03T16:43:27Z

Sources/SwiftFusion/Core/Manifold.swift

+  ///
+  /// Isomorphic to `R^n`, where `n` is the dimension of the manifold.
+  associatedtype LocalCoordinate: AdditiveArithmetic & Differentiable & VectorProtocol
+    where LocalCoordinate.TangentVector == LocalCoordinate


The where clause sounds redundant, as this is the case for any vector space. So, should that be of VectorProtocol?

Good point.

It can't currently be part of VectorProtocol because VectorProtocol does not imply Differentiable and so VectorProtocol can't specify constraints related to Differentiable concepts. (VectorProtocol does not imply Differentiable because some user might want to do vector calculations without opting in to all the extra baggage of differentiability.)

I can think of some ways to make this better, so I'll make a note of this and we can consider it when rethinking VectorProtocol.

@ProfFan and I found out that VectorProtocol inherits from AdditiveArithmetic, so maybe we can add the following protocol?

public protocol DifferentiableVector: Differentiable, VectorProtocol where Self.TangentVector == Self {}

and then

associatedtype LocalCoordinate : DifferentiableVector

I like the idea of a new protocol.

I'm discovering a lot of cases where we need euclidean vector operations over generic types (e.g. for the generic PriorFactor you need the squared norm of the error). How about we name it EuclideanVectorSpace and gradually add requirements over time as we need them?

I have added EuclideanVectorSpace to this PR, with just the requirements that you have suggested for now.

dellaert · 2020-05-03T16:46:02Z

Sources/SwiftFusion/Core/Manifold.swift

+  /// - There exists an open set `B` around `LocalCoordinate.zero` such that
+  ///   `local(global(b)) == b` for all `b \in B`.
+  @differentiable(wrt: local)
+  func global(_ local: LocalCoordinate) -> Self


suggest retract

I'll change later today!

dellaert · 2020-05-03T16:46:53Z

Sources/SwiftFusion/Core/Manifold.swift

+  /// - There exists an open set `B` around `self` such that `local(global(b)) == b` for all
+  ///   `b \in B`.
+  @differentiable(wrt: global)
+  func local(_ global: Self) -> LocalCoordinate


I think local works, or localCoordinates

I'll change later today!

dellaert · 2020-05-03T16:47:12Z

Sources/SwiftFusion/Core/Manifold.swift

+  /// The local coordinate system used in the chart.
+  ///
+  /// Isomorphic to `R^n`, where `n` is the dimension of the manifold.
+  associatedtype LocalCoordinate: AdditiveArithmetic & Differentiable & VectorProtocol


Why is LocalCoordinate not just TangentVector?

This is important, maybe everything will make sense if I can successfully explain why there is a ManifoldCoordinate.LocalCoordinate that is distinct from ManifoldCoordinate.TangentVector.

The high level idea behind this PR is:

When a user defines a struct, Swift can automatically generate a tangent space for the struct as the direct product of the struct's properties' tangent spaces. This is nice because derivatives involving the struct just work without any boilerplate.

For SwiftFusion's geometry, we want a different tangent space. We can achieve this by defining a fully custom TangentVector type (for an example, see Pose2.swift in the master branch as it is today). This involves an inconvenient amount of hard-to-explain boilerplate.

The tangent spaces in (1) and (2) are isomorphic to each other. This suggests a strategy for getting the advantages of (1) with the tangent space of (2): Do all the derivatives in the automatically-generated tangent space from (1), but map the derivatives through the isomorphism to (2) before exposing them to the user.

There are many ways to implement the strategy in (3), and this PR is one way. This PR works like this:

ManifoldCoordinate.TangentVector is generated automatically (1).

ManifoldCoordinate.LocalCoordinate is the user-specified tangent space (2).

ManifoldCoordinate.global/local (aka retract/localCoordinates) determine the isomorphism between (1) and (2).

Manifold is a wrapper type with Manifold.TangentVector == ManifoldCoordinate.LocalCoordinate, so that clients who use the Manifold wrapper type get the user-specified tangent space (2) without having to worry about translating between it and the automatically-generated tangent space (1).

dellaert · 2020-05-03T16:49:39Z

Sources/SwiftFusion/Core/Manifold.swift

+  /// The coordinate of `self`.
+  ///
+  /// Note: This is not differentiable, and therefore clients should use `coordinate` instead.
+  var coordinateStorage: Coordinate { get set }


I'm lost. Why have both coordinate and coordinateStorage? I suspect it's a workaround for something? Why can it not just be var coordinate : Coordinate {get set} here?

I think this is a workaround for the fact that currently public variables cannot have custom derivatives.

It is a workaround for the fact that protocol requirements cannot have default custom derivatives (https://bugs.swift.org/browse/TF-982).

dellaert · 2020-05-03T17:31:22Z

Sources/SwiftFusion/Core/Manifold.swift

+extension Manifold {
+  /// The coordinate of `self`.
+  @differentiable
+  public var coordinate: Coordinate { coordinateStorage }


This swift syntax I still don't understand, even after looking for it in manual: what does the { coordinateStorage } mean here?

This means coordinate is a computed property that, when accessed, will return coordinateStorage

Yeah, a recent swift feature is that single-expression function bodies implicitly return the value of the expression. I could start always using return if you think that would make things clearer.

I knew about that feature. It would indeed, I think, make it more clear to the novel user here, as this is a double whammy otherwise: computed property and this new feature. For example, I did not understand that this could not be used to set. Adding return makes it clear that this is just a gettable property (even though storage is get set)

added return!

dellaert · 2020-05-03T17:50:13Z

Sources/SwiftFusion/Core/Manifold.swift

+    // `x.coordinateStorage.global: LocalCoordinate -> Coordinate` defines _exactly_ how local
+    // coordinates at `x` map to global coordinates.
+    //
+    // Therefore, `D_x(f)` is the derivative of `x.coordinateStorage.global`.


Yes, but close the loop :-) Why does that mean that coordinateStorage.global is the pullback? And (see confusion above) why is it not coordinate.global ?

I have an idea how to update this whole comment to be clearer by using notation closer to the Swift code and by explicitly saying how we get the pullback expression:

// Explanation of this pullback: // // Let `f: Manifold -> Coordinate` be `f(x) = x.coordinateStorage`. // // `differential(at: x, in: f)` is a linear approximation of how changes in local // coordinates around `x` lead to changes in global coordinates around `x.coordinateStorage`. // // `x.coordinateStorage.global: LocalCoordinate -> Coordinate` defines _exactly_ how local // coordinates around zero map to global coordinates around `x`. // // Therefore, `differential(at: x, in: f) = differential(at: zero, in: x.coordinateStorage.global)`. // // The pullback is the dual map of the differential, so taking duals of both sides gives: // `pullback(at: x, in: f) = pullback(at: zero, in: x.coordinateStorage.global)`.

marcrasi

Thanks for the comments!

I have responded to all your questions.

I'll update the PR doc comments and function names a bit later today.

Here's an idea: This PR is currently approaching the problem by doing something that works today, with as many workarounds as necessary. Another way to approach the problem would be to write some "ideal world" code that doesn't actually work yet. This would let us talk about what we really want without getting distracted by workaround details. If you're interested, I could open up a parallel PR that does that.

marcrasi · 2020-05-04T21:01:09Z

Sources/SwiftFusion/Core/Manifold.swift

+  /// The local coordinate system used in the chart.
+  ///
+  /// Isomorphic to `R^n`, where `n` is the dimension of the manifold.
+  associatedtype LocalCoordinate: AdditiveArithmetic & Differentiable & VectorProtocol


This is important, maybe everything will make sense if I can successfully explain why there is a ManifoldCoordinate.LocalCoordinate that is distinct from ManifoldCoordinate.TangentVector.

The high level idea behind this PR is:

When a user defines a struct, Swift can automatically generate a tangent space for the struct as the direct product of the struct's properties' tangent spaces. This is nice because derivatives involving the struct just work without any boilerplate.

For SwiftFusion's geometry, we want a different tangent space. We can achieve this by defining a fully custom TangentVector type (for an example, see Pose2.swift in the master branch as it is today). This involves an inconvenient amount of hard-to-explain boilerplate.

The tangent spaces in (1) and (2) are isomorphic to each other. This suggests a strategy for getting the advantages of (1) with the tangent space of (2): Do all the derivatives in the automatically-generated tangent space from (1), but map the derivatives through the isomorphism to (2) before exposing them to the user.

There are many ways to implement the strategy in (3), and this PR is one way. This PR works like this:

ManifoldCoordinate.TangentVector is generated automatically (1).

ManifoldCoordinate.LocalCoordinate is the user-specified tangent space (2).

ManifoldCoordinate.global/local (aka retract/localCoordinates) determine the isomorphism between (1) and (2).

Manifold is a wrapper type with Manifold.TangentVector == ManifoldCoordinate.LocalCoordinate, so that clients who use the Manifold wrapper type get the user-specified tangent space (2) without having to worry about translating between it and the automatically-generated tangent space (1).

marcrasi · 2020-05-04T21:09:58Z

Sources/SwiftFusion/Core/Manifold.swift

+  ///
+  /// Isomorphic to `R^n`, where `n` is the dimension of the manifold.
+  associatedtype LocalCoordinate: AdditiveArithmetic & Differentiable & VectorProtocol
+    where LocalCoordinate.TangentVector == LocalCoordinate


Good point.

It can't currently be part of VectorProtocol because VectorProtocol does not imply Differentiable and so VectorProtocol can't specify constraints related to Differentiable concepts. (VectorProtocol does not imply Differentiable because some user might want to do vector calculations without opting in to all the extra baggage of differentiability.)

I can think of some ways to make this better, so I'll make a note of this and we can consider it when rethinking VectorProtocol.

marcrasi · 2020-05-04T21:10:08Z

Sources/SwiftFusion/Core/Manifold.swift

+  /// - There exists an open set `B` around `LocalCoordinate.zero` such that
+  ///   `local(global(b)) == b` for all `b \in B`.
+  @differentiable(wrt: local)
+  func global(_ local: LocalCoordinate) -> Self


I'll change later today!

marcrasi · 2020-05-04T21:10:22Z

Sources/SwiftFusion/Core/Manifold.swift

+  /// - There exists an open set `B` around `self` such that `local(global(b)) == b` for all
+  ///   `b \in B`.
+  @differentiable(wrt: global)
+  func local(_ global: Self) -> LocalCoordinate


I'll change later today!

marcrasi · 2020-05-04T21:13:09Z

Sources/SwiftFusion/Core/Manifold.swift

+  /// The coordinate of `self`.
+  ///
+  /// Note: This is not differentiable, and therefore clients should use `coordinate` instead.
+  var coordinateStorage: Coordinate { get set }


It is a workaround for the fact that protocol requirements cannot have default custom derivatives (https://bugs.swift.org/browse/TF-982).

marcrasi · 2020-05-04T21:14:16Z

Sources/SwiftFusion/Core/Manifold.swift

+extension Manifold {
+  /// The coordinate of `self`.
+  @differentiable
+  public var coordinate: Coordinate { coordinateStorage }


Yeah, a recent swift feature is that single-expression function bodies implicitly return the value of the expression. I could start always using return if you think that would make things clearer.

marcrasi · 2020-05-04T21:21:51Z

Sources/SwiftFusion/Core/Manifold.swift

+    // `x.coordinateStorage.global: LocalCoordinate -> Coordinate` defines _exactly_ how local
+    // coordinates at `x` map to global coordinates.
+    //
+    // Therefore, `D_x(f)` is the derivative of `x.coordinateStorage.global`.


I have an idea how to update this whole comment to be clearer by using notation closer to the Swift code and by explicitly saying how we get the pullback expression:

// Explanation of this pullback: // // Let `f: Manifold -> Coordinate` be `f(x) = x.coordinateStorage`. // // `differential(at: x, in: f)` is a linear approximation of how changes in local // coordinates around `x` lead to changes in global coordinates around `x.coordinateStorage`. // // `x.coordinateStorage.global: LocalCoordinate -> Coordinate` defines _exactly_ how local // coordinates around zero map to global coordinates around `x`. // // Therefore, `differential(at: x, in: f) = differential(at: zero, in: x.coordinateStorage.global)`. // // The pullback is the dual map of the differential, so taking duals of both sides gives: // `pullback(at: x, in: f) = pullback(at: zero, in: x.coordinateStorage.global)`.

marcrasi · 2020-05-07T17:59:54Z

By the way, I started a forum post on a small improvement to Swift AD motivated by this: https://forums.swift.org/t/stored-property-custom-derivatives/36194?u=marcrasi That change is pretty small and noncontroversial, so I expect it to happen soon.

dellaert

Made a lot of progress with @ProfFan on understanding. Most of my comments are geared towards making novel swift users grok this easily, as they are our prime target. Let's make sure to avoid magic where it really matters.

Still planning on reviewing the whole PR..

dellaert · 2020-05-07T19:07:36Z

Sources/SwiftFusion/Core/Manifold.swift

+  ///
+  /// Isomorphic to `R^n`, where `n` is the dimension of the manifold.
+  associatedtype LocalCoordinate: AdditiveArithmetic & Differentiable & VectorProtocol
+    where LocalCoordinate.TangentVector == LocalCoordinate


@ProfFan and I found out that VectorProtocol inherits from AdditiveArithmetic, so maybe we can add the following protocol?

public protocol DifferentiableVector: Differentiable, VectorProtocol where Self.TangentVector == Self {}

and then

associatedtype LocalCoordinate : DifferentiableVector

dellaert · 2020-05-07T19:35:14Z

Sources/SwiftFusion/Core/Manifold.swift

+public protocol Manifold: Differentiable {
+  /// The manifold's global coordinate system.
+  associatedtype Coordinate: ManifoldCoordinate
+      where Coordinate.LocalCoordinate == Self.TangentVector


It seems that this the only place is where Self.TangentVector is defined. But this does not read like a definition. A constraint that creates a type seems (to me) unnecessarily 'magical'.

Here is a proposal @ProfFan and I discussed:

associatedtype Coordinate: ManifoldCoordinate associatedtype TangentVector where TangentVector == Coordinate.LocalCoordinate

I don't know whether the above works, but the intent is to force the user to explicitly define the two associated types, which reinforces understanding without the user having to grok the (distributed and hard to follow) type inference process.

I can do something a bit different that I think accomplishes your goal. I moved the where Coordinate.LocalCoordinate == Self.TangentVector from here to the extension Manifold below. This makes it so that type inference can't figure out what Self.TangentVector should be, but you're still required to make it be equal to Coordinate.LocalCoordinate for the init(coordinate:) and .coordinate methods to work.

It's also important that the error messages when you forget to specify the TangentVector are easy to understand. I think that my solution makes them reasonably simple. This is what happens now if you try to define Rot2 without specifying TangentVector:

/usr/local/google/home/marcrasi/git/SwiftFusion/Sources/SwiftFusion/Geometry/Rot2.swift:27:30: error: property 'coordinate' requires the types 'Rot2.TangentVector' and 'Rot2Coordinate.LocalCoordinate' (aka 'Vector1') be equivalent public var theta: Double { coordinate.theta } ^

(It could be even better if we as library authors could specify custom error messages. Then we could make it say something like "You should add public typealias TangentVector = Vector1 to struct Rot2". But Swift doesn't have customizable error messages.)

dellaert · 2020-05-07T19:41:44Z

Sources/SwiftFusion/Geometry/Rot2.swift

-  public var c: Double {
-    c_
-  }
+  public var coordinateStorage: Rot2Coordinate


So I understand that this is where Coordinate is actually inferred. I would prefer modifying the recipe such that we ask the user (and we do it as well, to create good examples) to explicitly define Rot2Coordinate as Coordinate. @ProfFan tells me we can just say

associatedType Coordinate = Rot2Coordinate /// and TangentVector == Vector1

Note I added the comment about the TangentVector. I actually think it would be not be a bad thing if we forced the user to declare TangentVector as well, so the type constraint in Manifold.swift indeed becomes a constraint, not a definition (see my other comment).

I know the compiler can figure it out via type inference. What I am trying to accomplish is that we have a concise statement in every type that makes clear what the Coordinate/TangentVector types are. Hacking the user, not the compiler :-)

dellaert · 2020-05-07T20:05:28Z

Sources/SwiftFusion/Core/Manifold.swift

+extension Manifold {
+  /// The coordinate of `self`.
+  @differentiable
+  public var coordinate: Coordinate { coordinateStorage }


I knew about that feature. It would indeed, I think, make it more clear to the novel user here, as this is a double whammy otherwise: computed property and this new feature. For example, I did not understand that this could not be used to set. Adding return makes it clear that this is just a gettable property (even though storage is get set)

dellaert · 2020-05-07T20:09:59Z

Sources/SwiftFusion/Core/Manifold.swift

+    // `differential(at: x, in: f)` is a linear approximation of how changes in local
+    // coordinates around `x` lead to changes in global coordinates around `x.coordinateStorage`.
+    //
+    // `x.coordinateStorage.retract: LocalCoordinate -> Coordinate` defines _exactly_ how local


I think you should say here TangentVector -> Coordinate to be consistent with pullback above, and also because I think within Manifold we should really only talk about Coordinate/TangentVector once we 'defined` them.

dellaert · 2020-05-07T20:27:58Z

Sources/SwiftFusion/Geometry/Pose3.swift

+import TensorFlow
+
+/// TODO: Should be merged to Vector.swift
+public struct Vector6: Differentiable, VectorProtocol, KeyPathIterable, TangentStandardBasis {


Maybe Twist ? Definitely not a vector6 if you distinguish a v and w part.

dellaert · 2020-05-07T20:28:23Z

Sources/SwiftFusion/Geometry/Pose3.swift

+public extension Pose3Coordinate {
+  @differentiable
+  init(_ rot: Rot3, _ t: Vector3) {
+    self.t = t


respect the order, @ProfFan ! :-)

dellaert · 2020-05-07T20:28:36Z

Sources/SwiftFusion/Geometry/Pose3.swift

+// MARK: - Global Coordinate System
+
+public struct Pose3Coordinate: Equatable, KeyPathIterable {
+  var t: Vector3


respect the order, @ProfFan ! :-)

marcrasi · 2020-05-11T20:11:29Z

Thanks for the comments! I have responded inline and added a commit that addresses them.

I have also removed all the new manifold types (Pose2, Rot3, Pose3) from this PR to keep it small and focused. Those types also exist on other branches, and their implementations on other branches have started diverging, so I think this should make it easier to keep track of things.

ProfFan

I vote for squash and merging :)

marcrasi · 2020-05-11T20:48:06Z

Let's do it! I have resolved the conflict and I'll do a squash n' merge once the CI passes.

Of course, further discussion is still very welcome and I'll keep iterating on this if anyone has more thoughts.

dabrahams · 2020-08-30T16:34:32Z

doc/DifferentiableManifoldRecipe.md

+
+Create a `struct` whose values represent points on the manifold. Every point on
+the manifold must be representable by at least one `struct` value. Every
+`struct` value must represent at most one point on the manifold.


@marcrasi Doesn't this simply translate to: “Every point on the manifold must be representable by exactly one struct value?” If so, why not say it that way?

[I'm a little concerned about the idea that the values must cover the entire manifold (with no caveats), since obviously our numerical representations are limited.

dabrahams · 2020-08-30T16:35:55Z

doc/DifferentiableManifoldRecipe.md

+
+Create a `struct` whose values represent points on the manifold. Every point on
+the manifold must be representable by at least one `struct` value. Every
+`struct` value must represent at most one point on the manifold.


[I'm a little concerned about the idea that the values must cover the entire manifold (with no caveats), since obviously our numerical representations are limited.

dabrahams · 2020-08-30T16:37:31Z

doc/DifferentiableManifoldRecipe.md

+public protocol ManifoldCoordinate: Differentiable {
+  /// The local coordinate type of the manifold.
+  ///
+  /// This is the `TangentVector` of the `Manifold` wrapper type.


What “Manifold wrapper type?” I haven't seen one yet.

dabrahams · 2020-08-30T16:40:09Z

doc/DifferentiableManifoldRecipe.md

+
+Create a struct with a stored property and initalizer as follows:
+```
+var coordinateStorage: ${your coordinate type}


Don't think you should be using gyb syntax here 😉

It's unfortunate that we don't have a way to embed typographic styles into code snippets in markdown, or we could use italics. As it stands, I would just put "where X is your coordinate type” in the prose, and use “X”.

dabrahams · 2020-08-30T16:42:08Z

doc/DifferentiableManifoldRecipe.md

+```
+public typealias Coordinate = {your coordinate type}
+public typealias TangentVector = {your local coordinate type}
+```


This whole thing would be better as a copy/paste code template rather than fragments from the inside of some declaration not actually shown.

Marc Rasi added 2 commits April 28, 2020 18:25

initial work on Manifold

dec01ff

clean up manifold code and add a recipe document

6998f7e

dan-zheng reviewed Apr 29, 2020

View reviewed changes

doc/DifferentiableManifoldRecipe.md Outdated Show resolved Hide resolved

marcrasi added 2 commits April 29, 2020 09:47

fix doc comment

48eed76

update doc comment

d3d4c52

ProfFan mentioned this pull request Apr 29, 2020

Add Nonlinear FG interface #34

Closed

2 tasks

Migrated Pose2

b68b1e8

dellaert reviewed Apr 30, 2020

View reviewed changes

doc/DifferentiableManifoldRecipe.md Outdated Show resolved Hide resolved

marcrasi commented May 1, 2020

View reviewed changes

Update Pose2

c809e17

Minor change

a2a50c0

ProfFan requested a review from dellaert May 2, 2020 02:58

ProfFan added 2 commits May 1, 2020 23:27

Add Rot3, aka SO(3)

e638f38

Minor typo

8c42a78

Use the proper assertAllKeyPathEqual

1ee4d5b

Experimental Pose3 [NOT WORKING]

0e88ee9

fix for compiler error

c4070ed

Now with Pose3

d98e685

Comments

fe1e6b0

dellaert reviewed May 3, 2020

View reviewed changes

marcrasi commented May 4, 2020

View reviewed changes

update names and documentation comments

656d9de

Fix some typos

46b8985

dellaert reviewed May 7, 2020

View reviewed changes

ProfFan mentioned this pull request May 8, 2020

NLFG + Manifold Demo #44

Merged

2 tasks

marcrasi mentioned this pull request May 8, 2020

Ideal manifold definition experience #45

Open

Marc Rasi added 2 commits May 11, 2020 13:06

undo changes that exist on other branches

0a0c589

address usability comments

f526ea8

ProfFan approved these changes May 11, 2020

View reviewed changes

Merge branch 'master' into custom-manifold-type

87e6b74

marcrasi merged commit 6e92f77 into borglab:master May 11, 2020

marcrasi deleted the custom-manifold-type branch May 11, 2020 20:53

dabrahams reviewed Aug 30, 2020

View reviewed changes

Recipe for defining custom manifolds #37

Recipe for defining custom manifolds #37

Uh oh!

Conversation

marcrasi commented Apr 29, 2020

Uh oh!

Uh oh!

ProfFan commented Apr 29, 2020

Uh oh!

ProfFan commented Apr 30, 2020

Uh oh!

marcrasi commented Apr 30, 2020

Uh oh!

Uh oh!

marcrasi commented Apr 30, 2020

Uh oh!

dellaert commented Apr 30, 2020

Uh oh!

ProfFan commented May 1, 2020

Uh oh!

dellaert commented May 1, 2020

Uh oh!

dellaert commented May 1, 2020

Uh oh!

marcrasi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcrasi commented May 1, 2020

Uh oh!

marcrasi commented May 1, 2020

Uh oh!

ProfFan commented May 1, 2020

Uh oh!

marcrasi commented May 1, 2020

Uh oh!

dan-zheng commented May 2, 2020

Uh oh!

ProfFan commented May 2, 2020

Uh oh!

ProfFan commented May 2, 2020

Uh oh!

ProfFan commented May 2, 2020

Uh oh!

ProfFan commented May 2, 2020

Uh oh!

ProfFan commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dan-zheng commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ProfFan commented May 2, 2020

Uh oh!

ProfFan commented May 2, 2020

Uh oh!

marcrasi commented May 2, 2020

Uh oh!

ProfFan commented May 2, 2020

Uh oh!

dellaert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ProfFan commented May 2, 2020 •

edited

Loading

dan-zheng commented May 2, 2020 •

edited

Loading

marcrasi left a comment •

edited

Loading