Skip to content

Coot E01S03#3602

Merged
shrit merged 10 commits into
mlpack:masterfrom
shrit:coot_3
Jan 17, 2024
Merged

Coot E01S03#3602
shrit merged 10 commits into
mlpack:masterfrom
shrit:coot_3

Conversation

@shrit
Copy link
Copy Markdown
Member

@shrit shrit commented Jan 11, 2024

Prepare the following function signature for coot.
In the first PR, we will adapt it and add additional std::enable_if_t<IsCoot> to make work in that case.

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

shrit commented Jan 11, 2024

hmm, this is not throwing any error locally, probably C++ version issue

Signed-off-by: Omar Shrit <omar@avontech.fr>
Copy link
Copy Markdown
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, I think because the actual initialization functions use functions that are available in both Armadillo and Bandicoot that after this change things should work for Bandicoot too (once the IsMat and IsCube classes accept Bandicoot types, that is).

It looks like MSVC and the memory check job hiccup on the template SFINAE argument; I think the suggestion of typename = typename std::enable_if_t<...> should fix that. But, there are lots of ways to do it, template argument SFINAE is only one possibility (you could also use return type SFINAE or argument SFINAE...); up to you how you want to fix it, don't feel obligated to take my suggestion. 👍

Comment thread src/mlpack/methods/ann/init_rules/glorot_init.hpp Outdated
Comment thread src/mlpack/methods/ann/init_rules/glorot_init.hpp Outdated
Comment thread src/mlpack/core/util/arma_traits.hpp Outdated
Comment thread src/mlpack/methods/ann/init_rules/glorot_init.hpp Outdated
Comment thread src/mlpack/methods/ann/init_rules/glorot_init.hpp Outdated
Comment thread src/mlpack/methods/ann/init_rules/glorot_init.hpp Outdated
Signed-off-by: Omar Shrit <omar@avontech.fr>
@shrit
Copy link
Copy Markdown
Member Author

shrit commented Jan 14, 2024

This one should pass now, as it is the case on my machine.

Let us merge it asap.

Copy link
Copy Markdown
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.

Awesome, looks good to me. 👍

Comment thread src/mlpack/methods/ann/init_rules/glorot_init.hpp Outdated
Comment thread src/mlpack/methods/ann/init_rules/glorot_init.hpp Outdated
Comment thread src/mlpack/methods/ann/init_rules/glorot_init.hpp Outdated
Comment thread src/mlpack/methods/ann/init_rules/glorot_init.hpp Outdated
shrit and others added 4 commits January 15, 2024 17:15
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Copy link
Copy Markdown

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

Signed-off-by: Omar Shrit <omar@avontech.fr>
@shrit shrit merged commit e1b1357 into mlpack:master Jan 17, 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