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

QDA #386

Merged
merged 20 commits into from Mar 26, 2012
Merged

QDA #386

merged 20 commits into from Mar 26, 2012

Conversation

iglesias
Copy link
Collaborator

I messed a bit the previous pull request ( and my fork in general :S ) with commits that had nothing to do with QDA. I opened a new fork of shogun and have ordered the stuff properly.

Here it goes QDA. There are still a couple of comments in TODO dealing with a couple of doubts I have; they are just about things that probably could be done better but I was not really sure whether it pays off. I am waiting for your feedback :)


}

/* TODO avoid the repetition of the same operations here */
Copy link
Member

Choose a reason for hiding this comment

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

yeah you should really - precompute as much as is possible. then there is also no need to use the invsqrt approximation. so just store extra variables (or others processed).

@sonney2k
Copy link
Member

your patch looks very nice to me - just fix the remaining TODOs and double check that it works (e.g. run it through valgrind / compare results with the scikits implementation) and it is fine to be merged!

Thanks a lot!

@iglesias
Copy link
Collaborator Author

Here it is with the new changes we talked about. A quick summary:

  • I have moved the computations in apply that were only dependent of the
    training data to train_machine. This implies a couple of new class members
    and the removal of two class members that were used before in apply but
    now they're not necessary.
  • For the serialization issue we discussed in IRC. SGVector* members are
    now SGMatrix and SGMatrix* members, SGNDArray. I have extended a bit
    these classes SGMatrix and SGNDArray with some methods that are useful
    here.

Finally, the serialization of SGNDArray members is pending since you
suggested to leave it this way and think more carefully how this should be done.

Please let me know about your opinion :)

sonney2k pushed a commit that referenced this pull request Mar 26, 2012
@sonney2k sonney2k merged commit 97e8e21 into shogun-toolbox:master Mar 26, 2012
sonney2k pushed 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants