refactor LRSDP into separate LRSDPFunction class #305

Closed
rcurtin opened this Issue Dec 29, 2014 · 6 comments

Projects

None yet

1 participant

@rcurtin
Member
rcurtin commented Dec 29, 2014

Reported by rcurtin on 3 Nov 44079170 06:54 UTC
The LRSDP class (src/mlpack/core/optimizers/lrsdp/lrsdp.hpp) is an optimizer implementing a low-rank semidefinite program solver that uses the augmented Lagrangian optimizer (src/mlpack/core/optimizers/aug_langrangian/aug_lagrangian.hpp). The way the AugLagrangian class is set up is that it optimizes a AugLagrangianFunction class, and the AugLagrangianFunction class takes a FunctionType as a template parameter, and this FunctionType is the LRSDP class.

So you end up with a situation where LRSDP holds an object of type

AugLagrangian<LRSDP>

which in turn holds an object of type

AugLagrangianFunction<LRSDP>

A template parameter to AugLagrangianFunction must implement the following functions:

- double Evaluate()
- void Gradient(...)
- double EvaluateConstraint()
- void GradientConstraint(...)

So the LRSDP class must expose those four functions publicly for the whole thing to work. But this is confusing to users, who notice that the LRSDP class has these four functions, but they shouldn't call them, because those functions are only implemented as template specializations of AugLagrangianFunction::Evaluate() and AugLagrangianFunction::Gradient() (if you just call LRSDP::Evaluate(), it will throw an error).

A better class design could prevent this problem, by putting those four functions into a separate class, so that a user who creates an LRSDP object does not see those four functions.

@rcurtin rcurtin self-assigned this Dec 29, 2014
@rcurtin rcurtin closed this Dec 29, 2014
@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by abhishek on 3 May 44180012 05:57 UTC
If we replace the existing files in lrsdp with the above attachments. Then LRSDP object will not see the four functions.

  • double Evaluate()
  • void Gradient(...)
  • double EvaluateConstraint()
  • void GradientConstraint(...)

I have run the mlpack_test -t LRSDPTest and it is working fine with this.

Thanks,
Abhishek Laddha

@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by rcurtin on 11 Jun 44180374 00:34 UTC
Why is the implementation of LRSDPFunction in lrsdp_impl.hpp despite the fact that it is not a templated class?

@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by abhishek on 9 Sep 44181715 12:36 UTC
Thanks rcurtin for suggesting this.
We can put the implementation of LRSDPFunction lrsdp_function.cpp instead of lrsd_function_impl.hpp. It will be better because LRSDPFunction is not a template class. We can do it by making small changes

  1. rename the lrsdp_function_impl.cpp with lrsdp_function.cpp
  2. remove the line #include lrsdp_function_impl.hpp at the bottom of lrsdp_function.hpp
  3. Add following files in CMakeLists.txt
    lrsdp_function.hpp
    lrsdp_function.cpp
  4. Delete the following lines
    #ifndef __MLPACK_CORE_OPTIMIZERS_LRSDP_LRSDP_FUNCTION_IMPL_HPP
    #define __MLPACK_CORE_OPTIMIZERS_LRSDP_LRSDP_FUNCTION_IMPL_HPP

#endif

Similarly we can change lrsdp_impl.hpp to lrsdp.cpp which was already done in svn trunk.
I have attached the lrsdp.zip in the attachment with the changes.

Why lrsdp_impl.hpp is present if lrsdp.cpp already has implementation of Custom specializations of the AugmentedLagrangianFunction for the LRSDP case ?

Replying to rcurtin:

Why is the implementation of LRSDPFunction in lrsdp_impl.hpp despite the fact that it is not a templated class?

@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by rcurtin on 30 May 44240119 12:10 UTC
Hello Abhishek,

Thank you for the contribution. I have applied (a slightly modified version) in r16388. I would like to add your name and email to the list of contributors. Is that okay?

Thanks,

Ryan

@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by abhishek on 7 Jun 44240261 03:18 UTC
Replying to rcurtin:

Hello Abhishek,

Thank you for the contribution. I have applied (a slightly modified version) in r16388. I would like to add your name and email to the list of contributors. Is that okay?

Okay, That would be great. I will try to contribute more to mlpack.

Thanks,
Abhishek

Thanks,

Ryan

@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by rcurtin on 10 Jun 44240278 09:52 UTC
Added in r16391, and the website (http://www.mlpack.org/about.html) is also updated. Thanks again. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment