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

Added bmrmopt class in shogun/optimization/framework #2044

Closed
wants to merge 1 commit into from
Closed

Added bmrmopt class in shogun/optimization/framework #2044

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 20, 2014

@vigsterkr
The previous PR got corrupted for some reason.
This PR corresponds to issue #1969
bmrmopt is a simple class derived from libbmrm.cpp/h. This is the first class in shogun/optimization/framwork. Other bundle solvers can be easily derived from this class.
A brief overview of the files:
1.) functions.h/cpp - func class for handling functions and solution vectors
2.) constraints.h/cpp - constraints class for handling constraints. (For now only simplex constraints)
3.) CP.h/cpp - contains class for handling cutting planes
4.) optdefines.h - general include file
5.) bmrmopt.h/cpp - contains bmrmopt class (libbmrm.cpp). Other classes can be easily derived from this class. Will add them shortly in the same directory. (ppbmopt, p3bmopt, ncbmopt)

@ghost
Copy link
Author

ghost commented Mar 20, 2014

@vigsterkr Can you please look it? Build is already successful.

@vigsterkr
Copy link
Member

@kprah honestly i don't know how to write a comment on this that i don't feel that i'm being rude. I know that you are working hard to try to solve this problem, and this is your 3rd PR, which i really appreciate, but.....
but as the last 2 times i've told you it would have been much better to actually do some planning, i.e. have some discussion about how and what to do and then execute it. You've told me that you gonna do this this time, but again you've disappeared for couple of days and now you came back with this PR....

about the PR itself:
my main concern is not that you are not following any of Shogun's code and design patterns (which already a problem), but you don't follow any object oriented design pattern, which is very essential when you use C++ for coding. for example in all your classes everything is defined public, you create classes not for representing entities, but for representing functions...

i'm sorry if i offended you in any way, i'm really trying to do some constructive criticism here...

#include <shogun/optimization/framework/CP.h>
using namespace shogun;

CP::CP()
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to use initializer list here

@vigsterkr vigsterkr closed this Apr 22, 2014
@ghost ghost deleted the opti branch April 22, 2014 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants