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

Softtype as an instance or a type, T vs T() #577

Open
Affie opened this issue Aug 8, 2020 · 6 comments
Open

Softtype as an instance or a type, T vs T() #577

Affie opened this issue Aug 8, 2020 · 6 comments
Labels
design pattern VariableType This used to be called softtype (#603)
Milestone

Comments

@Affie
Copy link
Member

Affie commented Aug 8, 2020

Softtype is now a singleton type and to avoid confusion we have to decide whether it will be used as an instance or a type.
for example:
getManifolds(::Type{Pose2}) vs getManifolds(::Pose2)
For compatibility, I used an instance where @dehann used type getManifolds(vartype::Type{LinearConditional}) = (:Euclid,)

It doesn't seem to make a performance difference.

@Affie Affie added design pattern VariableType This used to be called softtype (#603) labels Aug 8, 2020
@Affie Affie added this to the v0.10.0 milestone Aug 8, 2020
@dehann
Copy link
Member

dehann commented Aug 8, 2020

I'd suggest only the ::Type{T}, i.e. addVariable!(..., Pose2). It's much cleaner and doesn't pollute with parenthesis everywhere.

@dehann
Copy link
Member

dehann commented Aug 8, 2020

its also easy to just do T() internally if needed, but its more difficult to go back obj --> ::Type{T}

@dehann
Copy link
Member

dehann commented Aug 13, 2020

Hi Johan, how do you feel about this?

const InstanceType{T} = Union{Type{<:T},T}

@Affie
Copy link
Member Author

Affie commented Aug 17, 2020

Hi Johan, how do you feel about this?

const InstanceType{T} = Union{Type{<:T},T}

I don't know how that will help. You need something like this discouraged pattern in the style guide:

foo(::Type{MyType}) = ..
foo(::MyType) = foo(MyType)

EDIT: Ok, I see how you use it in RoME. I was thinking of addVariable!

From a user side,

  • I think the type is easier as: addVariable!(..., Pose2)
  • I find functions of this form easier to work with: getManifolds(::Pose2) but I created a macro so one can generate and change it easily.

For now, my vote would be to just follow the style guide :

The preferred style is to use instances by default, and only add methods involving Type{MyType} later if they become necessary to solve some problem.

@Affie Affie modified the milestones: v0.11.0, v0.12.0 Dec 4, 2020
@dehann dehann modified the milestones: v0.12.0, v0.13.0 Feb 13, 2021
@dehann
Copy link
Member

dehann commented Jun 29, 2021

For now, my vote would be to just follow the style guide
I think the type is easier as: addVariable!(..., Pose2)

I'm also in the camp of using the type, but I don't know how this influences type stability. I'm assuming ::DataType is not stable hence the suggestion to work with instances... I think we can come back to this. This feels like a minor performance, but much larger user experience thing. Let's wait on a decision for now.

@dehann dehann modified the milestones: v0.15.0, v0.x.0 Jun 29, 2021
@Affie
Copy link
Member Author

Affie commented Jul 8, 2021

I came across this thread https://discourse.julialang.org/t/singleton-types-vs-instances-as-type-parameters/2802/11

Maybe it can be a tie breaker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design pattern VariableType This used to be called softtype (#603)
Projects
None yet
Development

No branches or pull requests

2 participants