Skip to content

Conversation

Affie
Copy link
Member

@Affie Affie commented Aug 27, 2021

close #1371

@codecov
Copy link

codecov bot commented Aug 28, 2021

Codecov Report

Merging #1373 (801b788) into master (bc4a731) will decrease coverage by 0.00%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1373      +/-   ##
==========================================
- Coverage   74.88%   74.88%   -0.01%     
==========================================
  Files          69       69              
  Lines        5001     5000       -1     
==========================================
- Hits         3745     3744       -1     
  Misses       1256     1256              
Impacted Files Coverage Δ
src/entities/FactorGradients.jl 100.00% <ø> (ø)
src/entities/FactorOperationalMemory.jl 100.00% <ø> (ø)
src/services/ApproxConv.jl 100.00% <ø> (ø)
src/Factors/Circular.jl 27.27% <50.00%> (ø)
src/DeconvUtils.jl 58.33% <75.00%> (-1.67%) ⬇️
src/ConsolidateParametricRelatives.jl 100.00% <100.00%> (ø)
src/FactorGraph.jl 71.78% <100.00%> (ø)
src/Factors/DefaultPrior.jl 52.94% <100.00%> (ø)
src/Factors/EuclidDistance.jl 41.66% <100.00%> (ø)
src/Factors/GenericFunctions.jl 60.97% <100.00%> (ø)
... and 11 more

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 bc4a731...801b788. Read the comment docs.

# build static lambda
unrollHypo! = if _slack === nothing
() -> cf( measurement_[smpid]..., (getindex.(varParams, smpid))... )
() -> cf( measurement_[smpid], (getindex.(varParams, smpid))... )
Copy link
Member

Choose a reason for hiding this comment

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

Why drop the tuple part of a measurement here? Code all over has residual functions follow
(cf)(z1,z2,x1,x2,x3)

seems like you restricting to only z1?

Copy link
Member

Choose a reason for hiding this comment

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

I take it the user must decompose the measurement sample data themselves inside the residual function, z1 here can be anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why drop the tuple part of a measurement here? Code all over has residual functions follow
(cf)(z1,z2,x1,x2,x3)

Only found DERelative that works this way and updated it.

I take it the user must decompose the measurement sample data themselves inside the residual function, z1 here can be anything.

yes, "one factor = one measurement". Measurement can be any type and it's up to the user to decide how to compose it. This way it's more flexible and we get rid of the tuples for simpler cases, this makes maintenance easier in my opinion.

@Affie Affie merged commit 1c577df into master Sep 8, 2021
@dehann dehann deleted the 21Q3/maint/rem_sample_tuple branch December 16, 2022 06:26
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.

Remove tuple requirement from getSample
2 participants