Factors renaming

Continuing with the renamings in wolf, the naming of factors is still an issue. We have 3 types of differentiation:

• Analytic
• Automatic
• Numeric I think that we don't have any numeric factor. We don't have a pure virtual class for that.

The naming of the factors should contain from which class they inherit from (following wolf naming rules): factor_{aut/ana}_xxx.h

Since "analytic" and "autodiff" is too long, I propose to put just 3 letters (or two) because with 4 we would have anal factors and it could be misleading XD

What does the naming guy (@jsola) think about this?

added Agreement required Idea labels

Two comments:

First, I like short names. And I like 3-letter shortcuts!

Second, I think keeping the full inheritance chain name is OK in some cases, but not in all cases.

In WOLF, a ProcessorBundleAdjustment is fine. A ProcessorTrackerLandmarkBundleAdjustment is the same processor, but its name is not fine.

So, what to do? I think there are two cases:

• classes that are not final. e.g. ProcessorTrackerFeature. Keep the inheritance chain.
• final classes. e.g. ProcessorOdom3d. Just put a name and forget about the whole chain.

For factors, we can indicate the type of differentiation in the name. Since the inheritance chain is not too long, OK. Having 3 letters is even better. But consider also removing fully the inheritance chain. e.g. FactorIMU, FactorOdom3d, etc, are all FactorAutodiff.

Needless to say (or is it needed? it is!), FactorPoseAnalytic is not good, and we need FactorAnalyticPose instead. In wolf, we do have FactorOdom2dAnalytic and FactorRelative2dAnalytic.

I think in this case, inheritance and differentiation type is the same. Some factors inherit from FactorAutodiff and others from FactorAnalytic.

Now, normally autodiff factors do not include autodiff in their name, and the analytic ones do (wrongly, as you pointed out). There are some exceptions.

My proposal is to add these three letters (just after "factor") but it is fine if we decide to make "aut" implicit. But we need to apply this rule for all factors.

I do not have a strong position on this.

I would keep names short.

In any case, analytic factors are only those that are very simple, because when things get complicated we always resort to autodiff. I mean, the functionality from the outside is the same, and we just indicate implementation details. Therefore, it is not so important that the name of the class reflects implementation details.

Other WOLF classes are different. The inheritance implies more logical differences (Motion, Tracker, etc) and are more important to appear in the name, maybe.

In view of this, I would consider treating the ddefault Factor type as autodiff, and do not mark the name with extra decorations. For the other Factors, I would use either 3-letter markers, or nothing.

But I am also fine with adding a 3-letter decorator to every factor.

I agree on the aim of keeping names short. The differentiation method is actually very important, since it determines the speed of the Jacobian evaluation.

Now, analytic factors are the strange ones. This is normal since they are more difficult to implement. However, if an analytic factor is available, the developer should definitely use it. Now it doesn't happen. Maybe the naming is a reason

One takes FactorOdom2d instead of FactorRelative2dAnalytic. I know the differentiation method is not only the difference but I guess it would happen anyway having FactorOdom2dAnalytic or FactorAnaOdom2d.

Maybe the "normal" ones (without markers) should be the analytic factors?

Yes that's a good point.

Then let us name all of them, because there's a variety of criteria. The user will not necessarily agree on our criterion for which of the factors has not the Jacobian method decorator in the name. So let us name them all.

The question is: if this is so important, should we keep the full name of the Jacobian method and not just 3 letters?

The only problem is the lenght, but we do this in the rest of cases (factor, not fac..).

Can we say that there is an agreement?

All names start with FactorAutodiff or FactorAnalytic (and eventually FactorNumeric).

I think not yet!

Sorry but I am reconsidering the arguments. In particular, I said that Auto / Analytic / Num are implementation details, you said yes, but that's important, and I said yes, that's important.

But, imagine we have a factor F that is coded with Autodiff. Then one day, we improve it with Analytic. The question is: once Analytic is out there running, why do we want to keep the Autodiff version? Analytic is just better, so it's an improvement over Autodiff, and Autodiff disappears from wolf.

In this sense, there is no need to differentiate names. Just substitute one for the other.

Good point. I didn't know if raising this issue... If we already have an analytic odom2d factor why do we need autodiff odom2d factor?

This action is quite radical.. I'm not sure enough.

Meeting agreement:

• Remove "analytic" or "autodiff" from names.
• Move autodiff factors (if analytic exists) to test/dummy folder (with "autodif" tag in the name).

removed Agreement required label

added To Do (release) label and removed Idea label

created merge request !359 (closed) to address this issue

mentioned in merge request !359 (closed)

assigned to @joanvallve

assigned to @jcasals and unassigned @joanvallve

More changes to be done:

Unify names for odometry factors

Now there are "Odom" "Relative" and "RelativePose" to refer to the same kind of factors.

I think we should move to the geometrical definition of the factor: "RelativePose". The same error definition can be used for an odometry and a loop closure.

For the naming, I propose that, if not explicitly defined, extrinsics are not considered in a "RelativePose" factor. Now, there is the "RelativePose2dWithExtrinsics" factor that already fulfills this.

getTopology()

While "RelativePose" is geometric meaning, the topology ("MOTION", "LOOP", etc) is semantic.

For example, now there are "FactorOdom2d" and "FactorOdom2dCloseloop". They are exactly equal except for this string returned by getTopology().

I imagine two options to address this repeated code:

1. Inheritance: Create derived classes FactorRelativePose2dLoop and FactorOdom2d from FactorRelativePose2d. Like the current implementation of FactorOdom2dAnalytic that inherits from FactorRelative2dAnalytic and only implements getTopology().
2. Attribute topology defined in the constructor (to be set by the processor that emplaces the factor).

I think both alternatives have advantages. 1 cleaner and elegant, 2 less files.

Opinions?

I would be in favor of the first option. I think that having a couple of light files is ok and, although the differences now may be small, I like that different things have different classes. Moreover, if we have distinct classes for different things, the compiler can help us make the distinction when it matters.

For instance, some computation maybe only makes sense semantically with closeloop factors but not with the other type. In this case, with separate classes you can make the compiler enforce this coherence, and you don't have to keep writing if statements and checking strings to make sure you don't mess up.

On the other hand, if this distinction will only ever be important for things like printing, maybe just having the processor fill the topology with the appropriate string is enough.

For now, it is just for visualization purpose. But I don't know if this distinction will be relevant for other things in the future.