Fix filenames or factor names in projection factors

Issue #56 new
Richard Roberts created an issue

It looks like currently we have the following:

  • GenericProjectionFactor defined in ProjectionFactor.h
  • GeneralSFMFactor defined in GeneralSFMFactor.h

I think one definite thing to do is make the class name for the first one match the filename (or vice versa).

But also we'd talked about perhaps changing the names in case you don't think they're logical as-is. But if you think the names make sense as-is then that's fine.

Comments (8)

  1. Frank Dellaert

    Yes, I always hated the confusion here. I don't like "SFM", General, or Generic. But I want something that is backwards compatible, if deprecated

  2. Chris Beall

    I agree, we can change the names. I think the original idea was to have all templated factors be General*. We can leave deprecated typedefs for a while.

    I propose: Don't rename ProjectionFactor.h, but rename class GenericProjectionFactor -> ProjectionFactor.

    Rename GeneralSFMFactor.cpp -> SFMFactor.cpp, rename classes SFMFactor, and SFMFactor2.

    How about also standardizing the constructor argument order for all factors to: Keys, Measurement(s), SharedNoiseModel, Calibration, optional body_transform, and deprecating the old ones?

  3. Frank Dellaert

    I like the argument order idea. But what is an SFMFactor? Why is different from a ProjectionFactor? I think they are all ProjectionFactors, no?

  4. Chris Beall

    The distinction (and the reason they ended up in separate files) is that SFMFactor optimizes for calibration, ProjectionFactor does not. The names don't make that clear. SFMFactor should instead be called ProjectionCalibrationFactor. So we have:

    • ProjectionFactor
    • ProjectionCalibrationFactor
  5. Frank Dellaert

    You're right. But, I would argue both are ProjectionFactors, just with different unknowns. We don't call the vanilla ProjectionFactor the ProjectionPointCameraFactor, so why add Calibration in there?

    This issue comes up in a number of other factors as well, BTW. I think the factors should be named after the measurement type, with variants somehow distinguished. We have not done this consistently. Sometimes using numbering, sometimes using special names. We could use the first letter of the arguments, e.g., ProjectionFactorPP, ProjectionFactorPPC, etc... Or numbering. What do you think?

  6. Chris Beall

    Oh, I actually like the first letter argument idea! Not a big fan of the numbering because it's not as informative, and in a few factors it implies cardinality of the underlying NoiseModelFactor (e.g. BoundingConstraint1,2), and in SFMFactor it does not (e.g. GeneralSFMFactor2: public NoiseModelFactor3).

  7. Richard Roberts reporter

    I like the first-letter idea too! I think doing it on all factors would get out of hand, but on the few that are ambiguous would be good.

  8. Log in to comment