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

coot 17: Refactor MakeAlias to make it compatible with bandicoot #3693

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

shrit
Copy link
Member

@shrit shrit commented Apr 19, 2024

No description provided.

Signed-off-by: Omar Shrit <omar@avontech.fr>
@shrit
Copy link
Member Author

shrit commented Apr 19, 2024

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>
Copy link
Member

@rcurtin rcurtin left a 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 Show resolved Hide resolved
src/mlpack/core/math/make_alias.hpp Outdated Show resolved Hide resolved
src/mlpack/core/math/make_alias.hpp Outdated Show resolved Hide resolved
const size_t numSlices,
const size_t offset = 0,
const bool strict = true,
const typename std::enable_if_t<IsCube<InCubeType>::value>* = 0)
Copy link
Member

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>.

Copy link
Member

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.

src/mlpack/methods/ann/layer/concat_impl.hpp Show resolved Hide resolved
shrit and others added 5 commits April 23, 2024 11:06
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>
Copy link
Member

@rcurtin rcurtin left a 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);
Copy link
Member

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.)

Copy link
Member

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.

src/mlpack/methods/ann/rnn_impl.hpp Outdated Show resolved Hide resolved
shrit and others added 3 commits April 26, 2024 17:22
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Signed-off-by: Omar Shrit <omar@avontech.fr>
Copy link
Member

@rcurtin rcurtin left a 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);
Copy link
Member

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
Copy link
Member

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)`
Copy link
Member

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?

Copy link
Member Author

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
Comment on lines 71 to 72
- If `offset` is `0` then make an identical alias, otherwise start making an
alias from a specific matrix location added to `offset`.
Copy link
Member

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?

Copy link
Member Author

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants