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

Templated Optimize() #113

Closed
wants to merge 46 commits into from

Conversation

@rcurtin
Copy link
Member

commented May 12, 2019

It's taken me a long time to put this together but this addresses #62 and provides a path for both optimizing sparse matrices (which is nice) and optimizing Bandicoot matrices (which is my real longer-term goal).

The short description of this PR is that now instead of having double Optimize(FunctionType& function, arma::mat& coordinates) as the signature of every optimizer, it's

template<typename FunctionType, typename MatType, typename GradType>
typename MatType::elem_type Optimize(FunctionType& function, MatType& coordinates);

instead. This allows the user to pass any type they want---that could be arma::mat, arma::fmat, arma::sp_mat, and so forth. By default, the type is checked for validity (which usually means it needs to be arma::mat, arma::fmat, arma::sp_mat, or arma::sp_fmat, but sometimes the sparse versions are disabled because they can't be used). But, type checking can be disabled by specifying the ENS_DISABLE_TYPE_CHECKS macro before including ensmallen.hpp. Static assertions are used to provide (somewhat) nice error messages.

The long description of this PR with individual changes highlighted to make review easier:

  1. All the Optimize() signatures changed, and in general I added tests to ensure that the Optimize() overloads would work with MatType set to arma::fmat or arma::sp_mat where applicable.

  2. In order to get the type checking right, each of the function forms like EvaluateForm, GradientForm, etc., had to be modified and put inside of a templated struct called TypedForms. So, now, the Function<> template also takes the MatType and GradType parameters, and so do the type checking functions like CheckNonDifferentiableFunctionTypeAPI() and others.

  3. For the sparse functionality, newer versions of Armadillo are needed so that sparse vectorise() and min() and max() are available. I simply put #ifdefs depending on the Armadillo version around the relevant tests instead of backporting that functionality.

  4. The SDP code needed some amount of overhaul for this to make sense. So the API has changed and so has the documentation. I deprecated the old versions; so they will still work until we remove them in ensmallen 2.10.0.

  5. I added a section to the documentation about templatized Optimize(), so users can realize that they can use any type. I will also add to the website's "Questions" section and perhaps one or two other places for visibility (not sure about that yet).

@zoq
zoq approved these changes May 26, 2019
Copy link
Member

left a comment

After the merge conflict is resolved this looks ready to me.

@zoq

This comment has been minimized.

Copy link
Member

commented May 27, 2019

I figured we should run the static analyses job before we merge this in as well.

@mlpack-bot
mlpack-bot bot approved these changes May 27, 2019
Copy link

left a comment

Second approval provided automatically after 24 hours. 👍

@zoq

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Wrong PR... I'll remove this one.

@zoq zoq force-pushed the rcurtin:templated_optimize branch from 9ab70e0 to 1a0b628 May 31, 2019

@zoq zoq referenced this pull request Jun 2, 2019
@rcurtin

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

The failure here in the tests should be resolved by #118.

@@ -104,7 +104,7 @@ typename MatType::elem_type CNE::Optimize(ArbitraryFunctionType& function,
<< std::endl;

// Find the fitness before optimization using given iterate parameters.
ElemType lastBestFitness = function.Evaluate(iterate);
size_t lastBestFitness = function.Evaluate(iterate);

This comment has been minimized.

Copy link
@zoq

zoq Jul 10, 2019

Member

I think using ElemTypemakes sense, the main issue is that the optimisation process stops if the current and previous fitness/loss is the same:

if (std::abs(lastBestFitness - fitnessValues.min()) < tolerance)

Depending on the problem this is more or less likely. As a workaround I used -1 as threshold in 2eda0cb, if we do this we can use a much lower population size and number of iterations as well.

This comment has been minimized.

Copy link
@rcurtin

rcurtin Jul 12, 2019

Author Member

Ah, ok. Since that commit is in #119, should we merge this as-is despite the failing build and then merge #119 just afterwards?

This comment has been minimized.

Copy link
@zoq

zoq Jul 12, 2019

Member

Are you talking about merging #113 as-is? Not sure, have to double check, but I think #119 does contain every commit from this PR, so I guess, it would be fine to just merge #119, once it is done (the StepTake additions is almost done)?

This comment has been minimized.

Copy link
@rcurtin

rcurtin Jul 12, 2019

Author Member

Oh, I see---yeah, we should just merge #119 all at once then. Maybe I made your life a little hard with the merge 3c72dae though; let me know if you want me to reproduce that merge on the callbacks branch (and cherry-pick a8a2ff0).

This comment has been minimized.

Copy link
@zoq

zoq Jul 13, 2019

Member

The last time I just used cherry-pick on your branch, so I think I can do the same for the latest commits, but let's see.

@mlpack-bot

This comment has been minimized.

Copy link

commented Aug 12, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍

@mlpack-bot mlpack-bot bot added the s: stale label Aug 12, 2019

@mlpack-bot mlpack-bot bot closed this Aug 19, 2019

@coatless

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@rcurtin + @zoq probably need to tweek the @mlpack-bot to avoid closing mlpack org member PRs?

@rcurtin

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Nah, actually in this case #119 supersedes this one so we can let it close. mlpack-bot did our job for us this time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.