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

Update MeanMaxPPE to a point on manifold #788

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Affie
Copy link
Member

@Affie Affie commented Jul 6, 2021

TODO serialization of MeanMaxPPE

@Affie Affie added enhancement New feature or request PPE manifolds labels Jul 6, 2021
@Affie Affie added this to the v0.15.1 milestone Jul 6, 2021
@Affie Affie requested a review from dehann July 6, 2021 11:05
@Affie Affie self-assigned this Jul 6, 2021
@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #788 (49426e5) into master (54c1289) will increase coverage by 5.64%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #788      +/-   ##
==========================================
+ Coverage   48.82%   54.47%   +5.64%     
==========================================
  Files          31       31              
  Lines        2597     2651      +54     
==========================================
+ Hits         1268     1444     +176     
+ Misses       1329     1207     -122     
Impacted Files Coverage Δ
src/services/CompareUtils.jl 19.35% <0.00%> (-0.78%) ⬇️
src/services/Serialization.jl 70.35% <0.00%> (ø)
src/entities/DFGVariable.jl 78.94% <100.00%> (ø)
src/Deprecated.jl 16.92% <0.00%> (-17.56%) ⬇️
src/DataBlobs/services/AbstractDataEntries.jl 96.77% <0.00%> (+0.34%) ⬆️
src/services/DFGVariable.jl 71.84% <0.00%> (+3.21%) ⬆️
src/LightDFG/FactorGraphs/BiMaps.jl 90.90% <0.00%> (+3.40%) ⬆️
src/services/DFGFactor.jl 86.36% <0.00%> (+4.54%) ⬆️
src/services/AbstractDFG.jl 82.95% <0.00%> (+10.57%) ⬆️
src/DataBlobs/services/BlobStores.jl 81.63% <0.00%> (+22.25%) ⬆️
... and 5 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 54c1289...49426e5. Read the comment docs.

@Affie
Copy link
Member Author

Affie commented Jul 7, 2021

What is the reason to add the variable type to ppe? won't it always be attached to a variable that has the type?

Also: why is the type included as a field and a type parameter? Is a type parameter not enough. Or a field only, but I don't know if ::Type is bad for performance.

@dehann
Copy link
Member

dehann commented Jul 20, 2021

What is the reason to add the variable type to ppe? won't it always be attached to a variable that has the type?

Previous conversation where we decided to duplicate on purpose, so that when a user gets a PPE in non-Julia world, they can back out what the values mean. PPE and VND might not always travel together.

Also: why is the type included as a field and a type parameter? Is a type parameter not enough. Or a field only,

Only should be good enough, lets perhaps track that in an issue and figure that decision out when the manifolds and IIF 1010 work simmers down a bit?

but I don't know if ::Type is bad for performance.

I also don't know. The first thing that comes to mind for me is if abstract types and type inference is needed when working with types. The second thing that comes to mind is that Base already uses the idea of Type for converters, eg convert(;:Type, obj) so it cannot be that bad for performance?

@dehann dehann modified the milestones: v0.15.1, v0.0.x Aug 23, 2021
@dehann
Copy link
Member

dehann commented Mar 28, 2022

This is sort of coming up, adding xref here for future design:

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