Skip to content

Conversation

GearsAD
Copy link
Collaborator

@GearsAD GearsAD commented Oct 14, 2019

…estimates

@GearsAD GearsAD requested review from Affie and dehann October 14, 2019 03:43
@GearsAD GearsAD mentioned this pull request Oct 14, 2019
5 tasks
@dehann
Copy link
Member

dehann commented Oct 14, 2019

Looks good to me!

EDIT: want to review Johan's suggestion during this cycle -- could probably do it sooner.

@dehann
Copy link
Member

dehann commented Oct 14, 2019

part of #147 discussion

@dehann
Copy link
Member

dehann commented Oct 14, 2019

want to make sure @Affie is happy before we merge the twig, and then to master

@codecov-io
Copy link

codecov-io commented Oct 14, 2019

Codecov Report

Merging #158 into feature/4Q19/issue147 will increase coverage by 0.08%.
The diff coverage is 83.33%.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           feature/4Q19/issue147     #158      +/-   ##
=========================================================
+ Coverage                  50.16%   50.25%   +0.08%     
=========================================================
  Files                         28       28              
  Lines                       1766     1769       +3     
=========================================================
+ Hits                         886      889       +3     
  Misses                       880      880
Impacted Files Coverage Δ
src/DistributedFactorGraphs.jl 100% <ø> (ø) ⬆️
src/services/DFGVariable.jl 25.71% <100%> (+3.32%) ⬆️
src/entities/DFGVariable.jl 76.66% <66.66%> (ø) ⬆️

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 bc139d6...e45186e. Read the comment docs.

@Affie
Copy link
Member

Affie commented Oct 14, 2019

You can go ahead, I'm not an expert when it comes to julia.
The only thing I've found with types represented as dictionaries is its harder to use.
Also please define the fields somehow (maybe in documentation), so I know for example :max will be there and the name won't change to something else. eg. :maximum

@dehann
Copy link
Member

dehann commented Oct 14, 2019

unlikely to have a name change from (eg :max to :maximum), but if it does occur we will do so with deprecation cycle and minor version numbers.

@dehann dehann added this to the v0.5.0 milestone Oct 14, 2019
Copy link
Member

@Affie Affie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just hold off on it not to break IIF and first do the branch. and this for v0.5.0

@dehann
Copy link
Member

dehann commented Oct 14, 2019

TLDR; hold off on merging this until we have spoken again please.

hi @GearsAD , Johan convinced me on using Dict{Symbol, <: AnAbstractType} given new and improved type inference with anonymous static parameters. #148 (comment)

So shall we change that now or later? I told Johan later, but I thought this was already merged. Perhaps we can just do it now before merging this. #148 already has it the way Johan suggested, we can maybe just introduce the name change to PointParametericEstimate etc.

timestamp::DateTime
tags::Vector{Symbol}
estimateDict::Dict{Symbol, Dict{Symbol, <: AbstractVariableEstimate}}
estimateDict::Dict{Symbol, PointParametricEstimates}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

estimateDict::Dict{Symbol, <: AbstractPointParametric}

or whatever the chosen name for AbstractPointParametric is in the end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from #148

<: AbstractVariableEstimate

ppeType::Symbol
estimate::Vector{Float64}
"""
struct PointParametricEstimates
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

struct PointParametricEstimates <: AbstractPointParametric as originally suggested by @Affie.

or whatever the chosen name for AbstractPointParametric is in the end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from #148

<: AbstractVariableEstimate

@Affie
Copy link
Member

Affie commented Oct 25, 2019

Closed by design decision from #147 last few comments

@Affie Affie closed this Oct 25, 2019
@Affie Affie deleted the twig/4Q19/issue147 branch August 31, 2020 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants