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
coot 17: Refactor MakeAlias to make it compatible with bandicoot #3693
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Omar Shrit <omar@avontech.fr>
not ready yet, just a prototype |
Signed-off-by: Omar Shrit <omar@avontech.fr>
Signed-off-by: Omar Shrit <omar@avontech.fr>
Signed-off-by: Omar Shrit <omar@avontech.fr>
Signed-off-by: Omar Shrit <omar@avontech.fr>
Signed-off-by: Omar Shrit <omar@avontech.fr>
Signed-off-by: Omar Shrit <omar@avontech.fr>
Signed-off-by: Omar Shrit <omar@avontech.fr>
Signed-off-by: Omar Shrit <omar@avontech.fr>
…passing yet Signed-off-by: Omar Shrit <omar@avontech.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed some of the changes, but I think I see what the problem is that is causing the tests to fail (I left a comment about it). Overall this will be a nice change, it's necessary that we not deal with direct memory pointers to be able to use Bandicoot (and it makes the code a bit cleaner too!).
src/mlpack/core/math/make_alias.hpp
Outdated
const size_t numSlices, | ||
const size_t offset = 0, | ||
const bool strict = true, | ||
const typename std::enable_if_t<IsCube<InCubeType>::value>* = 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want an enable_if
for all of these to ensure that eT
is the same for InCubeType
and OutCubeType
... for that matter, you could just keep the parameters as arma::Cube<eT>
. That might avoid some danger of wrong template type inference. This won't be a problem for Bandicoot, once we add a MakeAlias
version for coot::Mat<eT>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I now see, reading later, that we have some cases where we want to make a Cube
that is an alias of a Mat
. So I guess this does need to stay as a template parameter. But shouldn't we do SFINAE on OutCubeType
instead of InCubeType
? e.g. c
should always be a Cube
here, but it doesn't matter if oldCube
is actually a cube, since we are just taking its memory location.
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Co-authored-by: Ryan Curtin <ryan@ratml.org>
The input in some cases can be a matrix and the output should b a cube, if we are selecting the overload based on the input type then the offset will point to the wrong memeory location Signed-off-by: Omar Shrit <omar@avontech.fr>
Signed-off-by: Omar Shrit <omar@avontech.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks good to me! I think I found a minor bug, although the code does still work correctly. Do you think you can update the documentation for MakeAlias()
in doc/user/core.md
?
|
||
network[i]->SetWeights(weightsPtr + start); | ||
MatType tmpWeights; | ||
MakeAlias(tmpWeights, weightsIn, weightsIn.n_rows, weightsIn.n_cols, start); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the size here is wrong, I suspect it should be weightSize
x 1
or similar. (I think the code still works as-is, but it's probably better to fix it so that the alias we pass to network[i]
has size equal to how many weights network[i]
will use.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify what I mean is that the weights for the layer network[i]
are not of size weightsIn.n_rows x weightsIn.n_cols
---weightsIn
is the matrix that represents all the weights for all the layers in network
. For layer i
specifically, the size of the weights for that layer is weightSize
, with offset start
. So, the matrix tmpWeights
is too large. If you change it to MakeAlias(tmpWeights, weightsIn, weightSize, 1, start)
, I think it will be fine.
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Signed-off-by: Omar Shrit <omar@avontech.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks great to me; I think the only thing that remains is to update doc/user/core.md
's section on MakeAlias()
(it should be a pretty minor update).
responses.n_rows, batchSize); | ||
MakeAlias(responseData, responses.slice(responseStep), | ||
responses.n_rows, batchSize, | ||
begin * responses.slice(responseStep).n_rows); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just use responses.n_rows
here (and elsewhere in this file), since it's the same whether or not you take the slice.
} | ||
|
||
/** | ||
* Reconstruct `c` as an alias around the memory` newMem`, with size `numRows` x | ||
* `numCols` x `numSlices`. | ||
* Reconstruct `m` as an alias around the memory `newMem`, with size `numRows` x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch :)
Signed-off-by: Omar Shrit <omar@avontech.fr>
@@ -66,41 +66,25 @@ avoid copies. | |||
|
|||
--- | |||
|
|||
* `MakeAlias(a, mat, rows, cols, strict=true)` | |||
* `MakeAlias(a, mat, rows, cols, offset=0, strict=true)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document the version for vectors too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
doc/user/core.md
Outdated
- If `offset` is `0` then make an identical alias, otherwise start making an | ||
alias from a specific matrix location added to `offset`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a user might stumble on what offset
actually means (or what an 'identical alias' is). Maybe this or something like it might work?
If `offset` is `0`, then the alias is identical: the first element of `a` is the first element of `mat`.
Otherwise, the first element of `a` is the `offset`'th element of `mat`; elements in `mat` are
ordered in a [column-major way](../matrices.md#representing-data-in-mlpack).
That's just an idea, maybe it could be improved further?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more than good 👍
Signed-off-by: Omar Shrit <omar@avontech.fr>
No description provided.