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

shrit · 2024-04-19T18:33:56Z

No description provided.

Signed-off-by: Omar Shrit <omar@avontech.fr>

shrit · 2024-04-19T18:34:09Z

not ready yet, just a prototype

Signed-off-by: Omar Shrit <omar@avontech.fr>

…passing yet Signed-off-by: Omar Shrit <omar@avontech.fr>

rcurtin

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

rcurtin · 2024-04-22T19:33:42Z

src/mlpack/core/math/make_alias.hpp

+               const size_t numSlices,
+               const size_t offset = 0,
+               const bool strict = true,
+               const typename std::enable_if_t<IsCube<InCubeType>::value>* = 0)


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

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

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>

rcurtin

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?

rcurtin · 2024-04-24T18:14:41Z

src/mlpack/methods/ann/layer/multi_layer_impl.hpp

-
-    network[i]->SetWeights(weightsPtr + start);
+    MatType tmpWeights;
+    MakeAlias(tmpWeights, weightsIn, weightsIn.n_rows, weightsIn.n_cols, start);


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

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

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Signed-off-by: Omar Shrit <omar@avontech.fr>

rcurtin

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

rcurtin · 2024-05-07T23:16:50Z

src/mlpack/methods/ann/rnn_impl.hpp

-        responses.n_rows, batchSize);
+    MakeAlias(responseData, responses.slice(responseStep),
+        responses.n_rows, batchSize,
+        begin * responses.slice(responseStep).n_rows);


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.

rcurtin · 2024-05-07T23:17:28Z

src/mlpack/core/math/make_alias.hpp

 }

 /**
- * 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


Nice catch :)

Signed-off-by: Omar Shrit <omar@avontech.fr>

rcurtin · 2024-05-14T13:37:29Z

doc/user/core.md

@@ -66,41 +66,25 @@ avoid copies.

 ---

- * `MakeAlias(a, mat, rows, cols, strict=true)`
+ * `MakeAlias(a, mat, rows, cols, offset=0, strict=true)`


Can you document the version for vectors too?

rcurtin · 2024-05-14T13:52:36Z

doc/user/core.md

+   - If `offset` is `0` then make an identical alias, otherwise start making an
+     alias from a specific matrix location added to `offset`.


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?

This is more than good 👍

Signed-off-by: Omar Shrit <omar@avontech.fr>

rcurtin

Awesome, looks good to me. Just one minor trivial change. 👍

doc/user/core.md

Co-authored-by: Ryan Curtin <ryan@ratml.org>

mlpack-bot

Second approval provided automatically after 24 hours. 👍

Just a prototype for the new function signature

0415de3

Signed-off-by: Omar Shrit <omar@avontech.fr>

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Apr 19, 2024

shrit requested a review from rcurtin April 19, 2024 18:34

shrit added 8 commits April 19, 2024 21:36

Initial refactoring for a set of MakeAlias, not finished yet

ffb9c5f

Signed-off-by: Omar Shrit <omar@avontech.fr>

Fix SetWeight, and variable name

e303c5a

Signed-off-by: Omar Shrit <omar@avontech.fr>

Fix the SetWeights function globally

f9e3eba

Signed-off-by: Omar Shrit <omar@avontech.fr>

Fix const and SFINAE signature for all of these functions

4af7f48

Signed-off-by: Omar Shrit <omar@avontech.fr>

Remove memptr from tests

226acf5

Signed-off-by: Omar Shrit <omar@avontech.fr>

Regexp weights1.memptr() to wieghts1

de747c3

Signed-off-by: Omar Shrit <omar@avontech.fr>

Fix the missing parts inside method

04a3ed4

Signed-off-by: Omar Shrit <omar@avontech.fr>

Adding some forgetten refactoring around

09b3322

Signed-off-by: Omar Shrit <omar@avontech.fr>

shrit removed s: unanswered s: unlabeled labels Apr 20, 2024

Fix the slices MakeAlias, this is compiling on my end, tests are not …

7a363cb

…passing yet Signed-off-by: Omar Shrit <omar@avontech.fr>

rcurtin reviewed Apr 22, 2024

View reviewed changes

shrit and others added 5 commits April 23, 2024 11:06

Update src/mlpack/core/math/make_alias.hpp

d164b28

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/core/math/make_alias.hpp

b14a872

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/core/math/make_alias.hpp

5e98e3f

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Use the Output type with SFINAE

30c88bc

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>

Add tests are passing with no problem

3164cf6

Signed-off-by: Omar Shrit <omar@avontech.fr>

rcurtin reviewed Apr 24, 2024

View reviewed changes

shrit and others added 3 commits April 26, 2024 17:22

Update src/mlpack/methods/ann/rnn_impl.hpp

f229e4b

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Fix the function call

64a3900

Signed-off-by: Omar Shrit <omar@avontech.fr>

Merge branch 'coot_17' of github.com:shrit/mlpack into coot_17

5960f23

rcurtin reviewed May 7, 2024

View reviewed changes

Update Core with new MakeAlias signature

0f2bc5c

Signed-off-by: Omar Shrit <omar@avontech.fr>

rcurtin reviewed May 14, 2024

View reviewed changes

Fix the comments as requested

eef5b68

Signed-off-by: Omar Shrit <omar@avontech.fr>

rcurtin approved these changes May 14, 2024

View reviewed changes

doc/user/core.md Outdated Show resolved Hide resolved

Update doc/user/core.md

58970a5

Co-authored-by: Ryan Curtin <ryan@ratml.org>

mlpack-bot bot approved these changes May 15, 2024

View reviewed changes

shrit merged commit c219641 into mlpack:master May 16, 2024
20 checks passed

shrit deleted the coot_17 branch May 16, 2024 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shrit commented Apr 19, 2024

shrit commented Apr 19, 2024

rcurtin left a comment

rcurtin Apr 22, 2024

rcurtin Apr 22, 2024

rcurtin left a comment

rcurtin Apr 24, 2024

rcurtin May 3, 2024

rcurtin left a comment

rcurtin May 7, 2024

rcurtin May 7, 2024

rcurtin May 14, 2024

shrit May 14, 2024

rcurtin May 14, 2024

shrit May 14, 2024

rcurtin left a comment

mlpack-bot bot left a comment

		- If `offset` is `0` then make an identical alias, otherwise start making an
		alias from a specific matrix location added to `offset`.

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

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

Conversation

shrit commented Apr 19, 2024

shrit commented Apr 19, 2024

rcurtin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcurtin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcurtin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcurtin left a comment

Choose a reason for hiding this comment

mlpack-bot bot left a comment

Choose a reason for hiding this comment