Populate full variable and use ns time #823
Conversation
Codecov Report
@@ Coverage Diff @@
## master #823 +/- ##
==========================================
- Coverage 62.47% 62.34% -0.14%
==========================================
Files 39 39
Lines 4584 4600 +16
==========================================
+ Hits 2864 2868 +4
- Misses 1720 1732 +12
Continue to review full report at Codecov.
|
src/ApproxConv.jl
Outdated
| @@ -103,6 +103,8 @@ function prepareCommonConvWrapper!(ccwl::CommonConvWrapper{T}, | |||
| ccwl.cpt[i].p = collect(1:size(ccwl.cpt[i].X,1)) # collect(1:length(ccwl.cpt[i].Y)) | |||
| ccwl.cpt[i].Y = zeros(ccwl.xDim) # zeros(ccwl.partial ? length(ccwl.usrfnc!.partial) : ccwl.xDim ) | |||
| ccwl.cpt[i].res = zeros(ccwl.xDim) # used in ccw functor for AbstractRelativeFactorMinimize | |||
| # TODO JT - Confirm it should be updated here. | |||
| ccwl.cpt[i].factormetadata.fullvariables = copy(Xi) | |||
There was a problem hiding this comment.
is a copy necessary? Factor operations should not be allowed to change the variable metadata so we can just send the same reference, unless there is a stack vs. heap performance benefit somehow.
There was a problem hiding this comment.
Copy was to be on the save side. I want to be sure the container does not change. It looks like it doesn't, but there are some comments in the code about constructing Xi:
TODO -- this build up of Xi is excessive and could happen at addFactor time
Perhaps fullvairables can be populated at construction time?
There was a problem hiding this comment.
@dehann perhaps rather in prepgenericconvolution? Or will it not make a difference.
| @@ -608,6 +621,8 @@ function prepgenericconvolution( | |||
| ) | |||
| # | |||
| for i in 1:Threads.nthreads() | |||
| # TODO JT - Confirm it should be updated here. Also testing in prepareCommonConvWrapper! | |||
| ccw.cpt[i].factormetadata.fullvariables = copy(Xi) | |||
There was a problem hiding this comment.
again, not sure if the copy is really needed?
No description provided.