Remove FactorMetaData #1611
Remove FactorMetaData #1611
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1611 +/- ##
==========================================
- Coverage 77.52% 77.52% -0.01%
==========================================
Files 73 73
Lines 5433 5424 -9
==========================================
- Hits 4212 4205 -7
+ Misses 1221 1219 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
|
||
| ## TODO Consolidation WIP with FactorMetadata | ||
| # full list of variables connected to the factor | ||
| fullvariables::Vector{DFGVariable} |
There was a problem hiding this comment.
ah dynamic types. Let's do Vector just to get rid of FMd and the come back for {T<:Tuple} (separate PR)?
There was a problem hiding this comment.
Yes, currently it’s dispatched dynamically in any case and this way it at least saves on compile time.
But the plan is to figure out the best way after the cleanup and not waste time now.
| @@ -181,6 +153,8 @@ mutable struct CommonConvWrapper{ T<:AbstractFactor, | |||
| _gradients::G | |||
| # type used for cache | |||
| dummyCache::CT | |||
| # | |||
| fullvariables::Vector{DFGVariable} | |||
There was a problem hiding this comment.
yeah, cleanup of CCW after FMd and CPT are removed. That should start to help performance, allocations, dynamic dispatch etc. (separate PRs)
| cache = ccwl.dummyCache ) | ||
| cache = ccwl.dummyCache, | ||
| fullvariables=ccwl.fullvariables, | ||
| solvefor=ccwl.varidx) |
There was a problem hiding this comment.
we are also going to have to double check this with the multihypo recipes. Basically the ground rule is that a CalcFactor object represents one specific hypothesis. If there are particles with different hypos, then each particle gets a different CalcFactor object.
From that, .fullvariables and solvefor should only represent one active hypothesis choice for each CalcFactor object.
There was a problem hiding this comment.
should fullvariables be filtered by hypo
Yes it should be filtered by hypo
There was a problem hiding this comment.
Ok. I figured that much, but couldn’t quite see how it was done / can be done without first removing CPT.
Affie
force-pushed
the
22Q3/maint/rem_fmd
branch
from
a15c32f to
a55203c
Compare
|
We should probably tag a new patch release before merging this into master. |
Outstanding:
fullvariablesbe filtered by hyposolveforweirdness in DERelativeAlso, CPT should be removed in the same release to avoid possible issues.
This will close #1563