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

Factors return residual for CalcFactorParametric #1139

Closed

Conversation

Affie
Copy link
Member

@Affie Affie commented Jan 28, 2021

No description provided.

@Affie Affie changed the title Factors return resedual for CalcFactorParametric Factors return residual for CalcFactorParametric Jan 28, 2021
i missed this in an earlier cleanup,

cc @Affie
res .= z - (x2 - x1)
nothing
res = z - (x2 - x1)
return res
Copy link
Member

Choose a reason for hiding this comment

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

this looks good to me

Copy link
Member Author

Choose a reason for hiding this comment

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

LinearRelative is <:AbstractRelativeRoots so it needs residual for in-place NLSolve.
Is the plan to handle the residual in another place then?

Copy link
Member

@dehann dehann Jan 30, 2021

Choose a reason for hiding this comment

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

This is where residual is currently held (apologies res is too short a name, will fix at some point):

res::T # was Vector{Float64}

If it makes sense/helps, I have no problem moving residual container elsewhere; perhaps into CalcFactor or even as high as CommonConvWrapper (just remember to respect multithreading -- i.e. ccw.residual::Vector{RESIDUALTYPE}, one for each Threads.nthreads().

I'm not sure what the lifespan of ConvPerThread will be now that we have introduced CalcFactor and _CalcMalahanobisParametric. Also have to revisit FactorMetadata, so we can expect things to change in these parts over the coming weeks.

My suggestion for this general refactor/consolidation (bigger than just #467) would be small steps via PRs and Review that move quickly rather than trying one big jump.

@Affie Affie closed this Feb 5, 2021
@Affie Affie deleted the twig/21Q1/changeCalcFactorReturn branch February 9, 2021 14:53
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