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

Use CalcFactorNormSq as functor #1786

Merged
merged 4 commits into from
Oct 17, 2023
Merged

Use CalcFactorNormSq as functor #1786

merged 4 commits into from
Oct 17, 2023

Conversation

Affie
Copy link
Member

@Affie Affie commented Oct 13, 2023

This only focused on hex (pose2 and point2) a lot of other things are untested and likely broken.
Tests are looking ok.

This also fixes a deconv issue where the variable manifold was used instead of the factor manifold. and close #1785

Copy link
Member

@dehann dehann left a comment

Choose a reason for hiding this comment

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

Mostly looks good but want to talk through idea of each sample gets own CalcFactor with own .cache. This also so that the ensemble solve of points can easily be made multithreaded in future

@@ -43,7 +43,8 @@ function CalcFactorNormSq(
cache,
tuple(fullvariables...),
solvefor,
manifold
manifold,
ccwl.measurement,
Copy link
Member

Choose a reason for hiding this comment

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

This is all samples?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, currently

} <: CalcFactor{FT}
""" the interface compliant user object functor containing the data and logic """
factor::FT
""" what is the sample (particle) id for which the residual is being calculated """
_sampleIdx::Int
""" legacy support for variable values old functor residual functions.
TBD, this is still being used by DERelative factors. """
_legacyParams::X
_legacyParams::X #TODO rename to varValsHypo for consistent naming? and not not legacy any more
Copy link
Member

Choose a reason for hiding this comment

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

One CalcFactor per sample? History idea was CCW per solve and heap CalcFactor per sample. Then we can do multithreading for each sample. Also for more complicated factors that require own cache memory location. Don't want to index into cf.somemem[sampleIdx]


function _buildHypoCalcFactor(ccwl::CommonConvWrapper, smpid::Integer)
# build a view to the decision variable memory
varValsHypo = ccwl.varValsAll[][ccwl.hyporecipe.activehypo]
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, I just want talk through the one CalcFactor for each sample solve to be sure we're on the right track

@Affie
Copy link
Member Author

Affie commented Oct 13, 2023

A new calcfactor still gets created for each sample, but it currently shares some elements such as measurement.

@dehann
Copy link
Member

dehann commented Oct 14, 2023

A new calcfactor still gets created for each sample, but it currently shares some elements

Okay, great -- this looks good to me then thanks!

@Affie Affie force-pushed the 23Q4/enh/calcfactornormsq branch from 8276847 to 0888e90 Compare October 16, 2023 14:21
@Affie Affie force-pushed the 23Q4/enh/calcfactornormsq branch from cef4d52 to caa397a Compare October 17, 2023 07:17
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #1786 (986bdac) into master (c49e4b0) will increase coverage by 0.35%.
The diff coverage is 89.18%.

@@            Coverage Diff             @@
##           master    #1786      +/-   ##
==========================================
+ Coverage   75.95%   76.31%   +0.35%     
==========================================
  Files          82       82              
  Lines        5994     5975      -19     
==========================================
+ Hits         4553     4560       +7     
+ Misses       1441     1415      -26     
Files Coverage Δ
src/Factors/Mixture.jl 92.98% <ø> (ø)
src/entities/CalcFactor.jl 66.66% <ø> (ø)
src/services/CalcFactor.jl 71.51% <100.00%> (+0.18%) ⬆️
src/services/DeconvUtils.jl 72.30% <100.00%> (-0.42%) ⬇️
src/services/EvalFactor.jl 81.21% <100.00%> (ø)
src/services/NumericalCalculations.jl 87.61% <88.23%> (+17.91%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Affie Affie marked this pull request as ready for review October 17, 2023 09:01
@Affie
Copy link
Member Author

Affie commented Oct 17, 2023

Will do the same for deconv in a separate PR

@Affie
Copy link
Member Author

Affie commented Oct 17, 2023

NOTE _slack is not implemented for the new calc factor approach and will not work on ManifoldMinimize yet.
Should it or is autodiff the replacement?

@Affie Affie merged commit 65e1715 into master Oct 17, 2023
7 checks passed
@Affie Affie added this to the v0.35.0 milestone Oct 17, 2023
@Affie Affie mentioned this pull request Oct 20, 2023
3 tasks
@Affie Affie deleted the 23Q4/enh/calcfactornormsq branch May 6, 2024 06:35
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.

approxDeconv result wraps around on circular manifolds
2 participants