Fixed deprecated EigenMultivariateNormal and bumped cmake version

CMA-ES / libcmaes

libcmaes is a multithreaded C++11 library with Python bindings for high performance blackbox stochastic optimization using the CMA-ES algorithm for Covariance Matrix Adaptation Evolution Strategy

Other

321 stars 78 forks source link

Fixed deprecated EigenMultivariateNormal and bumped cmake version #226

Closed phbasler closed 3 years ago

phbasler commented 3 years ago

Included rule of 5 copy/move constructor for EigenMultivariateNormal
Bumped version to 0.9.8.1 (The 0.9.8 release did not have a cmake version increased. I took the freedom to bump it to 0.9.8.1 for cmake finds, feel free to revert it to 0.9.8.0 if you do not like that.)
The export as a cmake package did not work properly. I added the missing part and now find_package(libcmaes REQUIRED) works.

Closes #225

phbasler commented 3 years ago

Sorry for the typing changes in eigenmvn.h. Maybe you could add a .clang-format file for your repository

phbasler commented 3 years ago

@beniz I locally tried another approach for this: The problem was the scalar_normal_dist_op struct, defined in eigenmvn.h: If I changed it to a class and gave it user defined move constructors then it also works, without having to touch the EigenMultivariateNormal class.

Is there a reason why scalar_normal_dist_op needs to a struct instead of a class? If not I could update my PR for this approach.

beniz commented 3 years ago

Is there a reason why scalar_normal_dist_op needs to a struct instead of a class?

Not that I can remember, please PR the change.

Regarding any cmake changes, you can PR as well. And for clang-format, I use one for more recent projects, but if you have one handy, just PR as well.

Not that I don't want to do it myself, but it'll take much longer if I do...

phbasler commented 3 years ago

I updated the PR with the changes and the clang-format style we use in BSMPT.

After you accepted the PR, I would start a new PR where I just apply the clang-format to every file. This will be quite a few changes and it would be hard to differ the actual PR here from just formatting changes.

beniz commented 3 years ago

Hi @phbasler it'd be helpful to break this down into proper chunks (i.e. let's not mix version bump, clang, cmake and deprecated eigen code fix). Here is my suggestion:

leave this PR as is, I will take care of some cmake updates with commits directly (FYI, the cmake builds are not -O2 optimized, while the configure builds are)
please open another PR with just the eigen fix, if you can make it fit the current indentation by hand, that'd be great at this stage
i'll add the clang-format and reprocess, a bit later if you don't mind. Reason is I have a few small, long overdue commits to add, and I'll release a 0.10 or something.

My apologies for being slow on this.

phbasler commented 3 years ago

@beniz No problem, I will make a seperate PR for the eigen fix in a few hours. Should I make a new issue for the cmake fixes (e.g. the correct package export) or is this PR good enough as a reminder?

And no worries about the response/work speed. I find it fast enough :D

phbasler commented 3 years ago

@beniz You closed this, but the cmakelists.txt does not have the if(PROJECT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR) export(PACKAGE libcmaes) endif()

part. Do you commit this directly or should I PR it?

beniz commented 3 years ago

should I PR it?

My apologies @phbasler I missed this, yes a PR would be great, thanks!

phbasler commented 3 years ago

@beniz I noticed that the cmake export works fine on export. I was used that it is already exported if I just compile it. I can add that via PR if you want, but the export on install works fine.