address issue #147 (standard PPE) #148

dehann · 2019-10-10T01:53:38Z

resolves #147

codecov-io · 2019-10-10T02:13:39Z

Codecov Report

Merging #148 into master will increase coverage by 0.16%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
+ Coverage      50%   50.16%   +0.16%     
==========================================
  Files          28       28              
  Lines        1764     1766       +2     
==========================================
+ Hits          882      886       +4     
+ Misses        882      880       -2

Impacted Files	Coverage Δ
src/DistributedFactorGraphs.jl	`100% <ø> (ø)`	⬆️
src/entities/DFGVariable.jl	`76.66% <100%> (+1.66%)`	⬆️
src/services/DFGVariable.jl	`22.38% <100%> (ø)`	⬆️
src/Common.jl	`39.8% <33.33%> (+1.94%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2005021...bc139d6. Read the comment docs.

fix depreciation

Affie

See my changes for tests also

src/entities/DFGVariable.jl

dehann · 2019-10-10T15:02:57Z

hold off on merging till the AbstractVariableEstimate prototype is complete.

dehann · 2019-10-13T11:03:38Z

I'm really worried about performance hit of using <:AbstractVariableEstimate this way. DFGVariable is used a lot. Also not sure we want to template all of DFGVariable{T <: AbstractVariableEstimate} -- seems like a lot of excess in other places for something fairly small (PPE)?

Affie · 2019-10-13T15:37:54Z

I'm really worried about performance hit of using <:AbstractVariableEstimate this way. DFGVariable is used a lot. Also not sure we want to template all of DFGVariable{T <: AbstractVariableEstimate} -- seems like a lot of excess in other places for something fairly small (PPE)?

Sorry, I don’t understand. I didn’t make it a container of abstract variables because you were worried about performance. And didn't make parametric type, because i agree its unnecessary complications and restricting. Guess I need to learn some more julia then.

Is it because of the the second outer Dict?

dehann · 2019-10-14T02:59:56Z

I think we are close to a decision (Sam's suggestions seems like best mix of both). Dict of VariableEstimate containing a Dict). Mentioned on PR #148 and can discuss in person before merge.

I didn’t make it a container of abstract variables because you were worried about performance.

We can continue discussion post decision to make sure we are speaking the same language and that you are happy with the result -- when you did this in af107a6

estimateDict::Dict{Symbol, Dict{Symbol, <: AbstractVariableEstimate}}

isn't that a container of abstract variables?? It looks like an abstract container to me... or did i get lost somewhere along the way?

Affie · 2019-10-14T07:50:50Z

I don't mind the new changes sam is proposing. Just note they will be breaking, but rather sooner than later.
Just for my understanding of optimal code.
It is not abstract in this case:

abstract type AbstractTypeAB end

struct TypeA <: AbstractTypeAB
    a::Symbol
    b::Int
end

struct TypeB <: AbstractTypeAB
    a::Symbol
    b::Int
end

struct Option1 
    s::Symbol
    d::Dict{Symbol, AbstractTypeAB}
end

struct Option2 
    s::Symbol
    d::Dict{Symbol, <:AbstractTypeAB}
end

julia> o1 = Option1(:bla, Dict{Symbol,TypeA}())
Option1(:bla, Dict{Symbol,AbstractTypeAB}())

julia> push!(o1.d, :a=>TypeA(:a,1))
Dict{Symbol,AbstractTypeAB} with 1 entry:
  :a => TypeA(:a, 1)

julia> push!(o1.d, :b=>TypeB(:b,1))
Dict{Symbol,AbstractTypeAB} with 2 entries:
  :a => TypeA(:a, 1)
  :b => TypeB(:b, 1)

julia> o2 = Option2(:bla, Dict{Symbol,TypeA}())
Option2(:bla, Dict{Symbol,TypeA}())

julia> push!(o2.d, :a=>TypeA(:a,1))
Dict{Symbol,TypeA} with 1 entry:
  :a => TypeA(:a, 1)

julia> push!(o2.d, :b=>TypeB(:b,1))
ERROR: MethodError: Cannot `convert` an object of type TypeB to an object of type TypeA
Closest candidates are:
  convert(::Type{T}, ::T) where T at essentials.jl:167
  TypeA(::Any, ::Any) at untitled-b44b38864b48aa54882edecfec5246ba:5
Stacktrace:
 [1] setindex!(::Dict{Symbol,TypeA}, ::TypeB, ::Symbol) at ./dict.jl:380
 [2] push!(::Dict{Symbol,TypeA}, ::Pair{Symbol,TypeB}) at ./abstractdict.jl:479
 [3] top-level scope at none:0

But I guess you can force it with the second (outer) dict:

:default=>Dict{Symbol, VariableEstimate}} # can only contain VariableEstimate
:other=>Dict{Symbol, OtherVariableEstimateType}}#can only contain OtherVariableEstimateType
but Dict{Symbol, Dict{Symbol, <: AbstractVariableEstimate}}  would then contain both.

I still don't see the problem with it. I also think it happens in the Variables dictionary, since not all variables are of the same type. ie pose point

dehann · 2019-10-14T15:12:04Z

My opinion is that this is the part that is bad for performance:

Dict{Symbol, <: AbstractVariableEstimate}}

or

d::Dict{Symbol, AbstractTypeAB}

dehann · 2019-10-14T15:13:16Z

Functionally yes they will work, but that requires dynamic type inference.

Affie · 2019-10-14T15:15:43Z

Why is it different than templating? It also fixes the type when it is created, after the type cannot be changed and can only be of TypeA in this example.

dehann · 2019-10-14T15:25:34Z

Why is it different than templating?

Julia syntax specific thing.

`Option1` case

FROM ABOVE:

struct Option1 
    s::Symbol
    d::Dict{Symbol, AbstractTypeAB}
end

How does the compiler know in this case when you are going to do add a new type:

julia> push!(o1.d, :b=>TypeB(:b,1))
Dict{Symbol,AbstractTypeAB} with 2 entries:
  :a => TypeA(:a, 1)
  :b => TypeB(:b, 1)

hence dynamic type inference will always be needed, even if you just use it with TypeA. The very next one might be TypeB.

`Option2` case,

FROM ABOVE:

struct Option2 
    s::Symbol
    d::Dict{Symbol, <:AbstractTypeAB}
end

There is some wiggle room:

julia> o2 = Option2(:bla, Dict{Symbol,TypeA}())
Option2(:bla, Dict{Symbol,TypeA}())

julia> push!(o2.d, :a=>TypeA(:a,1))
Dict{Symbol,TypeA} with 1 entry:
  :a => TypeA(:a, 1)

julia> push!(o2.d, :b=>TypeB(:b,1))
ERROR: MethodError: Cannot `convert` an object of type TypeB to an object of type TypeA
Closest candidates are:
  convert(::Type{T}, ::T) where T at essentials.jl:167
  TypeA(::Any, ::Any) at untitled-b44b38864b48aa54882edecfec5246ba:5

I'm not sure if the compiler will interpret that as an anonymous static parameter or the same as Option1. They Julia guys will know. Perhaps I could ask for a higher opinion from @andreasnoack (Hi!) if he knows about Option1 vs Option2 in this example: #148 (comment)

Historically, I have always understood static type parameters (templating) as a requirement to avoid dynamic type inference -- i.e. Option3{T<:AbstractTypeAB}. The Option2 case could well be up to how successful static type inference at compile time is at resolving static parameters, and maybe newer Julia versions are better than previous versions.

Affie · 2019-10-14T15:39:58Z

Thanks, I think you finally get why I struggle to understand you.
I have it as an anonymous parameter, type generated is: d::Dict{Symbol,#s142} where #s142<:AbstractTypeAB

Affie · 2019-10-14T15:43:38Z

Maybe this is related:
https://docs.julialang.org/en/v1/manual/style-guide/#Don't-use-unnecessary-static-parameters-1

There is no performance difference

dehann · 2019-10-14T15:44:14Z

if anonymous parameter are reliable in all cases (not just small ones) --- e.g. #s142<:AbstractTypeAB --- then great i learned something, thanks! I do recall specifically this being a problem a couple months back, and i just haven't kept up with developments.

dehann · 2019-10-14T15:46:07Z

Maybe this is related:

Sure, but as before -- that is for function (aka member) definitions, not struct definitions. Two very different beasts. Yes, will be good if the language syntax is consistent. My push for explicit static parameter on struct is conservative for performance reasons.

dehann

I was not aware of anonymous static types in Julia, so Johan has convinced me of the alternative.

dehann · 2019-10-13T11:24:45Z

src/entities/DFGVariable.jl

    timestamp::DateTime
    tags::Vector{Symbol}
-    estimateDict::Dict{Symbol, Dict{Symbol, VariableEstimate}}
+    estimateDict::Dict{Symbol, Dict{Symbol, <: AbstractVariableEstimate}}


EDIT

know we have been going back and forth on this -- ~~but I'm not convinced about adding an abstract inheritance here unfortunately. Going to create more boiler plate at the top level for DFGVariable{T,P}. There are already 2 dictionaries here.~~

Will ask again why this is not simpler -- just make it a dictionary and be done?

estimateDict::Dict{Symbol, Dict{Symbol, Float64}}( :default => Dict{Symbol, Vector{Float64}}( :max => [1;2;3], :mean => [3;2;1], :modefit1 => [2;2;2], :modefit1_weight => [4;], ... :lastupdated => [2019;10;13;22;20;00.000] ))

Better solution

Can use anonymous static types which avoids the boiler plate. See Option 2 here and further:
#148 (comment)

easier to link discussions from Issue, so repeated here #147 (comment)

Affie · 2019-10-25T09:01:49Z

Closed by design decision from #147 last few comments

andreasnoack · 2019-10-28T15:11:58Z

Sorry for the slow reply here. Is the main worry the overhead from dynamic dispatch because the compiler can't infer the types or is the worry excessive compile time because of the use of type parameters? Let me try to modify the example slightly to highlight what's inferred and what is not. Here are three Option structs that all store dicts with Real values

julia> struct Option1
         x::Dict{Symbol,Real}
       end

julia> struct Option2
         x::Dict{Symbol,<:Real}
       end

julia> struct Option3{T}
         x::Dict{Symbol,T}
       end

To see what gets inferred and what doesn't, consider the two helper functions

julia> foo1(o) = o.x
foo1 (generic function with 1 method)

julia> foo2(d) = d[:a]
foo2 (generic function with 1 method)

julia> foo3(o) = o.x[:a]
foo3 (generic function with 1 method)

and the dict

julia> d = Dict(:a =>1)
Dict{Symbol,Int64} with 1 entry:
  :a => 1

From

julia> @code_warntype foo1(Option1(d))
Variables
  #self#::Core.Compiler.Const(foo1, false)
  o::Option1

Body::Dict{Symbol,Real}
1 ─ %1 = Base.getproperty(o, :x)::Dict{Symbol,Real}
└──      return %1

julia> @code_warntype foo1(Option2(d))
Variables
  #self#::Core.Compiler.Const(foo1, false)
  o::Option2

Body::Dict{Symbol,#s1} where #s1<:Real
1 ─ %1 = Base.getproperty(o, :x)::Dict{Symbol,#s1} where #s1<:Real
└──      return %1

julia> @code_warntype foo1(Option3(d))
Variables
  #self#::Core.Compiler.Const(foo1, false)
  o::Option3{Int64}

Body::Dict{Symbol,Int64}
1 ─ %1 = Base.getproperty(o, :x)::Dict{Symbol,Int64}
└──      return %1

we can see that 1 and 3 are inferred when calling foo1. However, from

julia> @code_warntype foo2(Option2(d).x)
Variables
  #self#::Core.Compiler.Const(foo2, false)
  d::Dict{Symbol,Int64}

Body::Int64
1 ─ %1 = Base.getindex(d, :a)::Int64
└──      return %1

julia> @code_warntype foo2(Option3(d).x)
Variables
  #self#::Core.Compiler.Const(foo2, false)
  d::Dict{Symbol,Int64}

Body::Int64
1 ─ %1 = Base.getindex(d, :a)::Int64
└──      return %1

we see that extracting the element from the dict is only inferred in 2 and 3. Finally, since failure of inference propagates we get

julia> @code_warntype foo3(Option1(d))
Variables
  #self#::Core.Compiler.Const(foo3, false)
  o::Option1

Body::Real
1 ─ %1 = Base.getproperty(o, :x)::Dict{Symbol,Real}
│   %2 = Base.getindex(%1, :a)::Real
└──      return %2

julia> @code_warntype foo3(Option2(d))
Variables
  #self#::Core.Compiler.Const(foo3, false)
  o::Option2

Body::Real
1 ─ %1 = Base.getproperty(o, :x)::Dict{Symbol,#s1} where #s1<:Real
│   %2 = Base.getindex(%1, :a)::Real
└──      return %2

julia> @code_warntype foo3(Option3(d))
Variables
  #self#::Core.Compiler.Const(foo3, false)
  o::Option3{Int64}

Body::Int64
1 ─ %1 = Base.getproperty(o, :x)::Dict{Symbol,Int64}
│   %2 = Base.getindex(%1, :a)::Int64
└──      return %2

i.e. only 3 is inferred.

If you want to avoid the dynamic dispatch, you should therefore use 3. However, it might not always be beneficial since it can cause extra compile time. In particular if the types can't be inferred. If the is a concern then it might actually be worth considering

julia> struct Option4
         x::Dict{Symbol,Any}
       end

since it might have the minimal compile overhead.

GearsAD · 2019-11-24T18:44:02Z

@andreasnoack Thanks for the great writeup!

address issue #147

1c7d729

dehann added the enhancement New feature or request label Oct 10, 2019

dehann requested a review from Affie October 10, 2019 01:53

dehann added the standardization label Oct 10, 2019

Affie added 2 commits October 10, 2019 10:16

added softtype(v::DFGVariable)

9fdac6b

Tests and export softtype,

b0a7699

fix depreciation

Affie approved these changes Oct 10, 2019

View reviewed changes

src/entities/DFGVariable.jl Outdated Show resolved Hide resolved

src/entities/DFGVariable.jl Show resolved Hide resolved

dehann mentioned this pull request Oct 10, 2019

Designing PPE VariableEstimate #147

Closed

5 tasks

dehann mentioned this pull request Oct 10, 2019

DFGVariable softtype field has abstract type #149

Closed

DFGVariable: estimateDict<: AbstractVariableEstimate

af107a6

Fixing conflict and fixing test

bc139d6

dehann modified the milestones: v0.4.1, v0.5.0 Oct 14, 2019

dehann commented Oct 14, 2019

View reviewed changes

dehann mentioned this pull request Oct 14, 2019

Add BigData #131

Closed

dehann mentioned this pull request Oct 14, 2019

Example using a blend of hard type metadata and dicts for adding new … #158

Closed

dehann added the DONT MERGE YET label Oct 16, 2019

This was referenced Oct 18, 2019

megamerge - trying to tie up loose ends #167

Merged

Closes #147 PointParametricEst implementation #183

Merged

Affie closed this Oct 25, 2019

Affie mentioned this pull request Nov 24, 2019

Variables and factors performance and parametric types #211

Closed

2 tasks

Affie deleted the feature/4Q19/issue147 branch August 31, 2020 09:20

Affie added performance type stability labels Aug 22, 2022

address issue #147 (standard PPE) #148

address issue #147 (standard PPE) #148

Uh oh!

Conversation

dehann commented Oct 10, 2019

Uh oh!

codecov-io commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Affie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dehann commented Oct 10, 2019

Uh oh!

dehann commented Oct 13, 2019

Uh oh!

Affie commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dehann commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Affie commented Oct 14, 2019

Uh oh!

dehann commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dehann commented Oct 14, 2019

Uh oh!

Affie commented Oct 14, 2019

Uh oh!

dehann commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Option1 case

Option2 case,

Uh oh!

Affie commented Oct 14, 2019

Uh oh!

Affie commented Oct 14, 2019

Uh oh!

dehann commented Oct 14, 2019

Uh oh!

dehann commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dehann left a comment

Choose a reason for hiding this comment

Uh oh!

dehann Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Better solution

Uh oh!

dehann Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

Affie commented Oct 25, 2019

Uh oh!

andreasnoack commented Oct 28, 2019

Uh oh!

GearsAD commented Nov 24, 2019

Uh oh!

Uh oh!

codecov-io commented Oct 10, 2019 •

edited

Loading

Affie commented Oct 13, 2019 •

edited

Loading

dehann commented Oct 14, 2019 •

edited

Loading

dehann commented Oct 14, 2019 •

edited

Loading

dehann commented Oct 14, 2019 •

edited

Loading

`Option1` case

`Option2` case,

dehann commented Oct 14, 2019 •

edited

Loading

dehann Oct 13, 2019 •

edited

Loading