Document core/math/
#3615
Document core/math/
#3615
Conversation
Whatever empirical improvements were seen in 2007 aren't seen today; a simpler implementation with std::max() and std::min() appears to always outperform by a relatively small margin. So, prefer the less-maintenance approach instead, and depend on the standard library.
…Col more carefully.
There was a problem hiding this comment.
Everything looks good, we should merge this as soon as possible.
Three minor observations:
- In this PR I can see
load.mdandindex.mdfiles but I remember that I have reviewed these previously, also if not I did not find the link in here. - We have the Mahalanobis distance, but it is not here, maybe not yet ?
- For the RBF kernel I did not know we could just compute it based on the distance, does this mean that it will work on any arbitrary distance or do we need to have the point vectors already declared, if this is the case we should mention that these are related.
| for (size_t j = 0; j < atoms; ++j) | ||
| { | ||
| if (arma::accu(codes.row(j) != 0) == 0) | ||
| inactiveAtoms.push_back(j); | ||
| if (arma::any(codes.row(j) != 0)) | ||
| activeAtoms.push_back((arma::uword) j); | ||
| } |
There was a problem hiding this comment.
I would just define j as arma::uword instead of casting it later, but eventually it should be the same
![mlpack-bot[bot]](https://avatars.githubusercontent.com/in/23934?s=60&v=4){kind=small}
Ha, then life happened to me... 😄
Those were actually part of #3603, which should be merged very shortly.
Yep, this class was actually documented as part of #3603, and in this PR I only took care of
Hm, I thought maybe this was already clear enough given how it's written---you can call |
|
I'll merge this one too once the build passes. Thanks for the review @shrit, sorry it took me so long to get back to 😄 |
2 participants
I went through and documented all the functions in
core/math/. The result is this updatedcore.mdpage where all examples are tested:https://www.ratml.org/misc/mlpack-markdown-doc/core.html
There were quite a number of changes I made here, and many things are removed:
ClampNonNegative(),ClampNonPositive(), andClampRange()are removed, preferring insteadstd::max()andstd::min(). That code dates back to ~2007 or so, and the code claimed empirical improvements. I couldn't reproduce any of them, and these implementations were generally noticeably slower than, say,std::max(lo, std::min(x, hi)).RemoveRows()can be expressed as a simple call to.rows()in a matrix, so I removed that. I had to refactorLocalCoordinateCodingandSparseCodingto track the active atoms instead of the inactive atoms as a result; basically,RemoveRows()let you give a vector of row indexes to remove, and.rows()requires the rows you want to keep. So I had to "invert" what we were keeping track of.VectorPower()was never used anywhere, so I removed it. (I forget what used it years ago.)Center()is so simple that I just inlined its implementation.WhitenUsingSVD(),Orthogonalize(),Svec(),SvecIndex(),SymKronId(), andSign()weren't used anywhere, so I removed them. I believe a couple of those are used in ensmallen and are in that codebase now.ObtainDistinctSamples()can be replaced witharma::randperm()and so it is now removed.I also made a couple small bugfixes:
Loading grayscale images had a bug; STB will return the actual number of channels in the image even if loading was done in grayscale, and in that case we shouldn't set
info.Channels(). See the STB documentation.ColumnsToBlockshad a minor offset calculation bug when the margin size is greater than 1 pixel. Easy fix.Lastly, I chose not to document
MakeAlias()at this time, because there is the outstanding issue of how it should be refactored, since there is another implementation (slightly different) of the same function insrc/mlpack/methods/ann/.