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

wip, consolidate factors and manifolds #1291

Closed
wants to merge 1 commit into from
Closed

Conversation

dehann
Copy link
Member

@dehann dehann commented Jul 5, 2021

I'd like to consolidate ManifoldPrior with Prior, but this will need:

  • converters to serialize manifolds (I was thinking of just doing it by name , ie shorthand string, and hard coding the converters, one for each manifold). e.g. "SpecialEuclidean(3)" gets persisted, and user code converts it back to whatever complicated type.
  • helper constructors so that legacy use of say Prior(Normal()) automatically inserts M=Euclidean(1),

EDIT @Affie , feel free to take over this branch in the morning hours there. I will be working on this again tomorrow.

@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #1291 (7943fa3) into master (9b0c5fc) will decrease coverage by 40.61%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #1291       +/-   ##
==========================================
- Coverage   40.63%   0.02%   -40.62%     
==========================================
  Files          57      57               
  Lines        4794    4821       +27     
==========================================
- Hits         1948       1     -1947     
- Misses       2846    4820     +1974     
Impacted Files Coverage Δ
src/ApproxConv.jl 0.00% <0.00%> (-66.55%) ⬇️
src/FactorGraph.jl 0.00% <0.00%> (-64.64%) ⬇️
src/Factors/DefaultPrior.jl 0.00% <0.00%> (-47.62%) ⬇️
src/Factors/GenericFunctions.jl 0.00% <ø> (ø)
src/IncrementalInference.jl 9.09% <ø> (-68.69%) ⬇️
src/entities/GraphConstraintTypes.jl 0.00% <ø> (ø)
src/FactorGraphTypes.jl 0.00% <0.00%> (-100.00%) ⬇️
src/GraphProductOperations.jl 0.00% <0.00%> (-95.24%) ⬇️
src/CalcFactor.jl 0.00% <0.00%> (-79.17%) ⬇️
src/TreeMessageAccessors.jl 0.00% <0.00%> (-76.00%) ⬇️
... and 32 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 9b0c5fc...7943fa3. Read the comment docs.

@Affie
Copy link
Member

Affie commented Jul 5, 2021

The manifolds prior part of this is currently in conflict with local changes I have as a wip to allow parametric and non-parametric to work with the same factors.
Can we hold off on consolidating Prior and ManifoldPrior for now?
I think we can serialize in the same way as with variables with a getManifold function if it's easier, but it is currently more convenient to just store the manifold in the factor.

@dehann
Copy link
Member Author

dehann commented Jul 5, 2021

okay, sure holding off on consolidation of Prior. I'm going to clear vecP stuff in RoME in the meantime then.

@Affie
Copy link
Member

Affie commented Jul 5, 2021

Hi, I've uploaded my local changes as is so long: #1292

dehann added a commit that referenced this pull request Jul 8, 2021
@dehann
Copy link
Member Author

dehann commented Jul 8, 2021

Replaced by 0f7edd4 in #1300

@dehann dehann closed this Jul 8, 2021
@dehann dehann deleted the 21Q3/enh/onmanientropy branch November 27, 2024 14:03
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.

2 participants