Skip to content

Conversation

@shrit
Copy link
Member

@shrit shrit commented Jan 10, 2024

This could be merged fast.

arma::repmat -> repmat
arma::randu -> randu

repmat function name only exist in Matlab, so no harm in removing arma,
Opencv has the same randu name so it could be tricky, especially if someone is doing object detection, unless if they use cv::randu in their code.

@shrit
Copy link
Member Author

shrit commented Jan 10, 2024

I thought for randu it would work, but it seems that the template parameter would not be considered in this context, only function parameters.

This should pass for repmat in this case

@shrit
Copy link
Member Author

shrit commented Jan 10, 2024

maybe someone needs to propose this to the C++ committee, to change ADL to ATDL 😛
We need to think about functions that do not take MatType as a parameter.
Hopefully we can merge this before Monday

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 changes look good to me (let's see if the tests pass, I think they should). The main comment I have is tedious: lots of the lines can be reflowed for easier readability now, since they are shorter.

void AddType<MatType>::Forward(const MatType& input, MatType& output)
{
output = input + arma::repmat(arma::vectorise(weights), 1, input.n_cols);
output = input + repmat(arma::vectorise(weights), 1, input.n_cols);
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 change arma::vectorise to vectorise too, or in another PR, up to you. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

in another PR, will make them simple short and fast

@rcurtin
Copy link
Member

rcurtin commented Jan 11, 2024

I suppose maybe we will need to make a SafeRandu() function or similar, that dispatches to Armadillo or Bandicoot accordingly. Perhaps sometime a long time from now we can use C++20: https://stackoverflow.com/questions/2953684/why-doesnt-adl-find-function-templates (apparently, ADL for functions with explicit template arguments works; I didn't check to see if it works right).

@rcurtin
Copy link
Member

rcurtin commented Jan 11, 2024

@mlpack-jenkins test this please

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.

Looks good. I found a bunch of places where we could still shorten lines (and one case where the line is too long). But, those are all small style concerns. Functionality-wise it looks great to me. 👍

shrit and others added 12 commits January 12, 2024 18:20
Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@shrit shrit merged commit a658940 into mlpack:master Jan 13, 2024
This was referenced May 26, 2024
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.

2 participants