Skip to content

Remove getSample tuple#1373

Merged
Affie merged 3 commits intomasterfrom
21Q3/maint/rem_sample_tuple
Sep 8, 2021
Merged

Remove getSample tuple#1373
Affie merged 3 commits intomasterfrom
21Q3/maint/rem_sample_tuple

Conversation

@Affie
Copy link
Copy Markdown
Member

@Affie Affie commented Aug 27, 2021

close #1371

Comment thread src/ODE/DERelative.jl
@codecov
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/ODE/DERelative.jl
@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