Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Refactoring of multiclass #458

Merged
merged 11 commits into from Apr 20, 2012

Conversation

Projects
None yet
3 participants
Contributor

pluskid commented Apr 16, 2012

Make MultiClassSVM derive from KernelMulticlassMachine.

However, there's a problem that I don't know how to solve: now MulticlassSVM is no longer a SVM, so in the GUIClassifier (I commented out several lines), it could no longer be held in a variable of type SVM*. Do you have any good ideas of how to fix this? I considered multiple inheritance, but this might rather makes things complicated.

@lisitsyn lisitsyn and 1 other commented on an outdated diff Apr 16, 2012

...ocumented/python_modular/classifier_larank_modular.py
print('LaRank')
- classifier_larank_modular(*parameter_list[0])
+ [predictions, svm, labels] = classifier_larank_modular(*parameter_list[0])
+ if len(argv) > 2:
@lisitsyn

lisitsyn Apr 16, 2012

Owner

Please remove that later

@pluskid

pluskid Apr 16, 2012

Contributor

Thanks, I have removed this. :)

Owner

lisitsyn commented Apr 16, 2012

Great huge work. I'll check things again a little bit later

Owner

sonney2k commented Apr 17, 2012

thanks - we cannot use multiple inheritance though. so either we have multiclassmachine -> multiclasskernelmachine
or kernelmachine -> multiclassmachine but I prefer the first option so one should probably use CMachine instead of CSVM and do appropriate casts. to do so maybe we need similar to what we have for features (feature properties) machine properties that then could be set to linear, multiclass, kernel or so.

Contributor

pluskid commented Apr 17, 2012

The variable is a constraint_generator for MKL. The code for MKL seems to require a CSVM * for constraint_generator. However, I don't know much about MKL, so I'm not sure whether multi-class SVM can be used as a constraint_generator in MKL. If it is true, we might also modify MKL to use CMachine and cast to either CSVM or CMulticlassSVM, but this could be ugly.

Adopting similar ways as those used in features could be good solution. But I think that would be a bunch of refactoring work. Shall we open an issue on the issue tracker or a thread in the mailing list to discuss about this problem?

@sonney2k sonney2k and 1 other commented on an outdated diff Apr 17, 2012

src/shogun/classifier/svm/MultiClassSVM.cpp
- m_svms=SG_MALLOC(CSVM*, m_num_svms);
- if (m_svms)
- {
- for (index_t i=0; i<m_num_svms; ++i)
- m_svms[i]=NULL;
+ m_machines = SGVector<CMachine *>(num_svms);
@sonney2k

sonney2k Apr 17, 2012

Owner

it is not possible to have an SGVector of some non-numeric type, use DynArray or DynamicArray or DynamicObjectArray instead.

@pluskid

pluskid Apr 18, 2012

Contributor

Hi, why impossible? Is it a convention? I followed that of CMulticlassMachine, so shall we also change that in CMulticlassMachine?

@sonney2k

sonney2k Apr 18, 2012

Owner

Yes it is a convention - SGVector is meant to be able to SGVector a,b; a+b; a*b and other operations... So it was already a bug in the previous code.

@pluskid

pluskid Apr 19, 2012

Contributor

OK, I will fix this tomorrow.

@pluskid

pluskid Apr 20, 2012

Contributor

@sonney2k There's a small problem switching from SGVector to DynArray. It's mainly due to MulticlassMachine::register_parameters, who need to register the m_machines variable. However, DynArray is a more encapsulated class than SGVector: it's memory buffer and size variable is not directly acceptable from outside. I don't think making them public or making MulticlassMachine a friend of DynArray good ideas. Do you have any suggestion here?

@sonney2k

sonney2k Apr 20, 2012

Owner

Use CDynamicObjectArray.

@pluskid

pluskid Apr 20, 2012

Contributor

@sonney2k thanks! CDyamicObjectArray seems to be a better choice, with automatic REF/UNREF. However, the problem is still there. How should this statement be re-written when we use CDynamicObjectArray for m_machines:

m_parameters->add_vector((CSGObject***)&m_machines.vector,&m_machines.vlen, "m_machines");

this is in the register_parameters function of CMulticlassMachine.

@sonney2k

sonney2k Apr 20, 2012

Owner
SG_ADD(&m_machines, "machines", "Machines that jointly make up the multi-class machine.")
@pluskid

pluskid Apr 20, 2012

Contributor

Thanks, I have fixed this. :)

Now please review again for merging.

@sonney2k sonney2k and 1 other commented on an outdated diff Apr 17, 2012

src/shogun/classifier/svm/MultiClassSVM.cpp
SG_ERROR("Multiclass SVM not trained!\n");
SG_INFO( "Writing model file...");
fprintf(modelfl,"%%MultiClassSVM\n");
- fprintf(modelfl,"multiclass_type=%d;\n", multiclass_type);
- fprintf(modelfl,"num_classes=%d;\n", m_num_classes);
- fprintf(modelfl,"num_svms=%d;\n", m_num_svms);
- fprintf(modelfl,"kernel='%s';\n", kernel->get_name());
+ //fprintf(modelfl,"multiclass_type=%d;\n", multiclass_type);
@sonney2k

sonney2k Apr 17, 2012

Owner

why is this line commented out?

@pluskid

pluskid Apr 18, 2012

Contributor

Because multiclass_type is no longer a variable of CMulticlassSVM, but one variable of his base class CMulticlassMachine. I think it might be his base class's responsibility to write and load this variable. However, I'm not sure about this, so I commented out instead of deleting it. If you think it should still be read/write here, I can uncomment this and change "multiclass_type" to "m_multiclass_strategy". :)

@sonney2k sonney2k and 1 other commented on an outdated diff Apr 17, 2012

src/shogun/ui/GUIClassifier.cpp
else if (strcmp(name,"GMNPSVM")==0)
{
SG_UNREF(constraint_generator);
constraint_generator= new CGMNPSVM();
SG_INFO("created GMNP-SVM object\n") ;
}
+ */
@sonney2k

sonney2k Apr 17, 2012

Owner

just remove the commented parts - it is not legal to use multiclass svms as MKL constraint generators (at least curently)

@pluskid

pluskid Apr 18, 2012

Contributor

Thanks! I've deleted these code.

Contributor

pluskid commented Apr 18, 2012

BTW, I scanned the IRC log, blackburn is right for why I'm making MulticlassSVM a friend of SVM: I need to access some protected variable of CSVM (actually, base class of CSVM), svs here. In a hurry, have to leave now, will discuss with you after I come back (or if I could find internet access there ;) )

Owner

lisitsyn commented Apr 18, 2012

Last thing that should be done here (for me) is to move MultiClassSVM to multiclass folder (and all its descendants) and rename it to Multi_c_lassSVM

Contributor

pluskid commented Apr 19, 2012

@lisitsyn that was what I initially did -- and failed with some strange error of the SWIG python modular. However, I think later I figured out the reason is that the order of the "include" statement in the .i files matters very much. So I'll fix this again, tomorrow. Just back from the trip, too tired to work this night. :)

Contributor

pluskid commented Apr 20, 2012

@lisitsyn oops, I didn't see the "(for me)", so I do the naming convention fixing. I also moved CMulticlassSVM to multiclass folder. But I didn't move its descendants. You can do that if you think necessary (after this is merged).

Owner

lisitsyn commented Apr 20, 2012

It is really huge! However I've checked the changes and they are ok for me - great work!

sonney2k added a commit that referenced this pull request Apr 20, 2012

@sonney2k sonney2k merged commit 48b3ffd into shogun-toolbox:master Apr 20, 2012

sonney2k added a commit that referenced this pull request May 22, 2012

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