Coot 9: Change `arma::conv_to` to `conv_to` #3614

shrit · 2024-01-30T12:05:27Z

This PR uses the same class that @zoq did for ensmallen I copy pasted it and changed the relevant information for mlpack.
Then applied the modification on the ann part and a couple of places where we are using generic MatType with no nested template types.

PS: I will open subsequent PRs for randi, randn, and the remaining functions.

Signed-off-by: Omar Shrit <[email protected]>

src/mlpack/methods/adaboost/adaboost_main.cpp

src/mlpack/core/util/conv_to.hpp

Signed-off-by: Omar Shrit <[email protected]>

shrit · 2024-01-30T21:31:20Z

src/mlpack/core/util/conv_to.hpp

+   inline static OutputType from(const InputType& input,
+                                 const typename std::enable_if_t<
+                                 coot::is_coot_type<InputType>::value ||
+                                 coot::is_coot_type<OutputType>::value>* = 0)


@rcurtin now we are looking at both input and output types to cover cases such as std::vector<T>

Otherwise this is ready for review 👍

Signed-off-by: Omar Shrit <[email protected]>

shrit · 2024-01-30T22:04:18Z

src/mlpack/methods/preprocess/image_converter_main.cpp

 * http://www.opensource.org/licenses/BSD-3-Clause for more information.
 */
 #include <mlpack/prereqs.hpp>
-#include <mlpack/core/util/io.hpp>


we should not call any of these directly but instead, we should call core.hpp

You could remove the prereqs.hpp too, since core.hpp includes it.

shrit · 2024-01-30T22:05:35Z

src/mlpack/prereqs.hpp

-// All code should have access to logging.
-#include <mlpack/core/util/log.hpp>
-#include <mlpack/core/util/io.hpp>
-


These are already defined in core.hpp so they are useless here, and they are pulling load_image too early when it is not needed.
I just removed them it should be fine

I think that's ok, so long as it doesn't make us include core.hpp where we could previously only include prereqs.hpp.

Signed-off-by: Omar Shrit <[email protected]>

shrit · 2024-01-30T23:22:53Z

okay, I do not understand why the CI is failing, I am not having this issue locally at all. So either it is a CMake or a compiler issue or something wrong with our setups

rcurtin · 2024-01-30T23:29:52Z

The header include ordering is very carefully balanced. If you change it, I'm not entirely surprised that things break. I would revert to the minimal changes necessary for this PR (i.e. no include changes at all) and go from there.

shrit · 2024-01-31T11:47:08Z

actually, headers are not breaking, because this is passing on my machine without an issue, and I am surprised that this is causing an error.

This reverts commit 447a9cc. Signed-off-by: Omar Shrit <[email protected]>

rcurtin

Looks good to me. Sorry it took so long for me to return to this! It has been a really busy two weeks with some paper deadlines.

rcurtin · 2024-02-10T17:59:07Z

src/mlpack/core/util/conv_to.hpp

+    * @param input The input that is converted.
+    */
+   template<typename InputType>
+   inline static OutputType from(const InputType& input,


It's pedantic, I know, but to match the style guide this should be From(). If you want I can supply a patch, I know it's a little tedious to change.

src/mlpack/methods/adaboost/adaboost_main.cpp

rcurtin · 2024-02-10T18:01:45Z

src/mlpack/methods/lsh/lsh_search_impl.hpp

      // the primary hash table).
      hashMat(arma::span(1, T), i) = // Compute code of rows 1:end of column i
-        arma::conv_to< arma::Col<size_t> >:: // floor by typecasting to size_t
+        ConvTo< arma::Col<size_t> >:: // floor by typecasting to size_t


Suggested change

ConvTo< arma::Col<size_t> >:: // floor by typecasting to size_t

ConvTo<arma::Col<size_t>>:: // floor by typecasting to size_t

I think the extra spaces were here because it was pre-C++11...

rcurtin · 2024-02-10T18:03:33Z

src/mlpack/methods/preprocess/image_converter_main.cpp

 * http://www.opensource.org/licenses/BSD-3-Clause for more information.
 */
 #include <mlpack/prereqs.hpp>
-#include <mlpack/core/util/io.hpp>


You could remove the prereqs.hpp too, since core.hpp includes it.

rcurtin · 2024-02-10T18:09:21Z

src/mlpack/prereqs.hpp

-// All code should have access to logging.
-#include <mlpack/core/util/log.hpp>
-#include <mlpack/core/util/io.hpp>
-


I think that's ok, so long as it doesn't make us include core.hpp where we could previously only include prereqs.hpp.

Signed-off-by: Omar Shrit <[email protected]>

mlpack-bot

Second approval provided automatically after 24 hours. 👍

shrit added 4 commits January 30, 2024 12:45

Add the conv_to file from ensmallen

655afe8

Signed-off-by: Omar Shrit <[email protected]>

Use conv_to class instead of arma::conv_to

d48b265

Signed-off-by: Omar Shrit <[email protected]>

Fix compiler ambiguity related to not using arma::

82122b9

Signed-off-by: Omar Shrit <[email protected]>

Change arma::conv_to to conv_to

71506bb

Signed-off-by: Omar Shrit <[email protected]>

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Jan 30, 2024

shrit requested a review from rcurtin January 30, 2024 12:05

shrit removed s: unanswered s: unlabeled labels Jan 30, 2024

shrit commented Jan 30, 2024

View reviewed changes

src/mlpack/methods/adaboost/adaboost_main.cpp Outdated Show resolved Hide resolved

rcurtin reviewed Jan 30, 2024

View reviewed changes

src/mlpack/core/util/conv_to.hpp Outdated Show resolved Hide resolved

shrit added 6 commits January 30, 2024 22:00

Change conv_to to ConvTo

8c31348

Signed-off-by: Omar Shrit <[email protected]>

Fix for input and output types e.g., std::vector

7225f54

Signed-off-by: Omar Shrit <[email protected]>

Change to ConvTo in lars

251202b

Signed-off-by: Omar Shrit <[email protected]>

Change to ConvTo in all methods

a91ffc7

Signed-off-by: Omar Shrit <[email protected]>

Change to ConvTo in tests

56827ba

Signed-off-by: Omar Shrit <[email protected]>

Change in core as well for Conv_to

06c9cbf

Signed-off-by: Omar Shrit <[email protected]>

shrit commented Jan 30, 2024

View reviewed changes

shrit added 2 commits January 30, 2024 23:02

Fix header order that can lead to an issue

29f1d75

Signed-off-by: Omar Shrit <[email protected]>

Remove it completely instead of commenting

46329d1

Signed-off-by: Omar Shrit <[email protected]>

shrit commented Jan 30, 2024

View reviewed changes

shrit added 3 commits January 30, 2024 23:10

Let us see if this will work

f2b9319

Signed-off-by: Omar Shrit <[email protected]>

Refactor to have a unique util.hpp

447a9cc

Signed-off-by: Omar Shrit <[email protected]>

Let see if this is enough for Julia to pass

19290b3

Signed-off-by: Omar Shrit <[email protected]>

Revert "Refactor to have a unique util.hpp"

53eee8a

This reverts commit 447a9cc. Signed-off-by: Omar Shrit <[email protected]>

rcurtin approved these changes Feb 10, 2024

View reviewed changes

shrit added 2 commits February 11, 2024 16:33

Change from to From in method

e88c623

Signed-off-by: Omar Shrit <[email protected]>

Change from to From in tests and core

4f77f2f

Signed-off-by: Omar Shrit <[email protected]>

mlpack-bot bot approved these changes Feb 11, 2024

View reviewed changes

mlpack-bot bot removed the s: needs review label Feb 11, 2024

shrit merged commit 3ec1716 into mlpack:master Feb 12, 2024

shrit deleted the coot_9 branch February 12, 2024 11:38

This was referenced May 26, 2024

Release version 4.4.0 #3716

Closed

Release version 4.4.0 #3717

Merged

	ConvTo< arma::Col<size_t> >:: // floor by typecasting to size_t
	ConvTo<arma::Col<size_t>>:: // floor by typecasting to size_t

Uh oh!

Coot 9: Change arma::conv_to to conv_to #3614

Coot 9: Change arma::conv_to to conv_to #3614

Uh oh!

Conversation

shrit commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shrit commented Jan 30, 2024

Uh oh!

rcurtin commented Jan 30, 2024

Uh oh!

shrit commented Jan 31, 2024

Uh oh!

rcurtin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlpack-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Coot 9: Change `arma::conv_to` to `conv_to` #3614

Coot 9: Change `arma::conv_to` to `conv_to` #3614

shrit commented Jan 30, 2024 •

edited

Loading