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

Deprecation, rename softtype -> VariableType #603

Closed
dehann opened this issue Aug 13, 2020 · 4 comments · Fixed by #700
Closed

Deprecation, rename softtype -> VariableType #603

dehann opened this issue Aug 13, 2020 · 4 comments · Fixed by #700

Comments

@dehann
Copy link
Member

dehann commented Aug 13, 2020

the idea of softtype has been removed, and is now just a regular hard type, so I think we should change the naming convention to VariableType. Some of the API will need to be changed too, e.g. getSofttype will become getVariableType which is then similar to the existing getFactorType.

Also, the name softtype doesn't have any meaning really. I added it a long time back for a different reason and use of the terminology softtype should probably expire. The motivation was to allow different FactorsTypes to be added to the same VariableType, which increases reuse -- e.g. MultivariateEuclid{2} can be used on a Point2, Pose2, Pose3, Point3. Becomes even more useful for quick tricks like PartialEuclid{...} (if you want to regularize one among variables).

Related:

@dehann dehann added API types standardization VariableType This used to be called softtype (#603) labels Aug 13, 2020
@dehann dehann added this to the v0.11.0 milestone Aug 13, 2020
@dehann dehann changed the title Deprecate naming softType -> VariableType Deprecate naming softtype -> VariableType Aug 13, 2020
@dehann
Copy link
Member Author

dehann commented Aug 14, 2020

@Affie
Copy link
Member

Affie commented Aug 20, 2020

I like getVariableType to be consistent with factors, so this sounds like a good idea.

I just want to make sure i understand the intention. For me the only thing that changed in softtype was making it “constant” (no more variable fields). So can’t see why this(The motivation was to allow different FactorsTypes to be added to the same VariableType, which increases reuse) will no longer work.
Is the intention to drop the above reuse?
I guess manifold.jl will change softtype in any case.

@dehann
Copy link
Member Author

dehann commented Oct 1, 2020

So can’t see why this(The motivation was to allow different FactorsTypes to be added to the same VariableType, which increases reuse) will no longer work.

That should still work. I'm just suggesting we rename softtype --> VariableType

Is the intention to drop the above reuse?

The current does not enforce that say a Pose2 variable is required when connecting say a Pose2Pose2 factor -- so the reusability and "mix-and-match" flavor of the code remains in tact. But you are right to ask, that is what I was trying to communicate when we decided to do JuliaRobotics/IncrementalInference.jl#783 .

I think we are in a good place except for the naming. Initially "mix-andmatch" was confused with "softtype". Then came system requirements wanting "Manifest" information. "Manifest information" is the same as "hard-typing" in this case. So changing the name to VariableType will help avoid the confusion, but "mix-and-match" will still work because we don't enforce "type-on-type" graph construction.

We should probably add a checking function or suppressible warning when "type-on-type" discrepancies occur. Although i do think "type-on-type" purity is too strong a requirement. Note other libraries like iSAM or GTSAM do require "type-on-type" purity (my understanding, although I have not check that as fact). I'm trying to keep an open mind by not looking at any of the other libraries.

I guess manifold.jl will change softtype in any case.

Likely yes, and that might be a better time to at least enforce some basic necessary (but not sufficient) conditions that manifolds between variable and factor dimensions should at least be compatible (don't have to be exactly the same)

@dehann dehann changed the title Deprecate naming softtype -> VariableType Deprecation, rename softtype -> VariableType Oct 1, 2020
@dehann
Copy link
Member Author

dehann commented Nov 30, 2020

HI @Affie , just a reminder if you have a few minutes spare perhaps please. Lets target this for DFG v0.11.0.

The serialization enhancement likely to only happen for v0.12.0 or v0.13.0 -- there are all sorts of small issues and ideas trickling in for that so probably best to keep major serialization updates on back burner for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants