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 static check terms to match documentation #145

Merged
merged 3 commits into from Dec 13, 2019

Conversation

@rcurtin
Copy link
Member

rcurtin commented Dec 10, 2019

This solves #144. The original code for the static function type checks used the terms decomposable and non-differentiable, but in the documentation we use separable and arbitrary to represent those two concepts. This PR changes all of the code to match the terms in the documentation.

@zoq zoq removed the s: unanswered label Dec 10, 2019
typename MatType,
typename... CallbackTypes>
typename MatType::elem_type Optimize(DecomposableFunctionType& function,
typename MatType::elem_type Optimize(SeparableFunctionType& function,

This comment has been minimized.

Copy link
@zoq

zoq Dec 10, 2019

Member

Maybe it makes sense to rewrite the calculation of the first objective:

ElemType currentObjective = 0;
for (size_t f = 0; f < numFunctions; f += batchSize)
{
const size_t effectiveBatchSize = std::min(batchSize, numFunctions - f);
const ElemType objective = function.Evaluate(mPosition[0], f,
effectiveBatchSize);
currentObjective += objective;
Callback::Evaluate(*this, function, mPosition[0], objective,
callbacks...);
}

and use the ArbitraryFunctionTypeAPI as well. Let me know what you think, I can open a PR.

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 11, 2019

Author Member

It looks like RandomSelection depends on the objective function being separable:

double Select(DecomposableFunctionType& function,

So maybe we shouldn't? I guess another idea would be to make Function provide non-separable overloads of Evaluate() for separable functions (which I've been meaning to do for a while anyway).

This comment has been minimized.

Copy link
@zoq

zoq Dec 11, 2019

Member

You are right, another overload sounds like the right thing to do. I think we should open another PR for that, what do you think?

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 11, 2019

Author Member

Sure, that works. Do you want me to make the code changes and open the PR, or do you want to?

This comment has been minimized.

Copy link
@zoq

zoq Dec 11, 2019

Member

I can open a PR in the next days, unless you like to, probably not urgent.

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 12, 2019

Author Member

Sounds good, I'll focus on other PRs like #138.

This comment has been minimized.

Copy link
@zoq

zoq Dec 12, 2019

Member

Sounds good.

@zoq
zoq approved these changes Dec 12, 2019
Copy link
Member

zoq left a comment

Looks good to me.

@mlpack-bot
mlpack-bot bot approved these changes Dec 13, 2019
Copy link

mlpack-bot bot left a comment

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit e4b8397 into mlpack:master Dec 13, 2019
3 checks passed
3 checks passed
Static Code Analysis Checks Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mlpack master build test Build finished.
Details
@rcurtin rcurtin deleted the rcurtin:de-doc-fix branch Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.