Remove namespaces #3269

rcurtin · 2022-09-09T01:01:58Z

This PR implements the result of the discussion in #3261:

All namespaces under mlpack:: are now removed, except mlpack::data and mlpack::util. mlpack::data remains because it is very specific to utilities for loading/saving data, and mlpack::util is utility stuff that shouldn't ever really be touched by users anyway, so it makes sense to keep it separate. mlpack::bindings is also left as-is, because users shouldn't ever be using things in that namespace.
All using namespace ... declarations in test files and examples are updated accordingly; only using namespace mlpack should be needed.
The namespace_compat.hpp file is added, which reintroduces reverse-compatibility namespaces like this:

namespace mlpack {
namespace math { using namespace mlpack; }
}

That can help make transitions for users to mlpack 4 a little less painful.

A few classes needed to be renamed here and there to avoid collisions.

shrit · 2022-09-09T10:15:11Z

Okay, finished 10 percent of this PR and still a lot of things to do, I will continue later this weekend

rcurtin · 2022-09-09T18:52:45Z

Okay, finished 10 percent of this PR and still a lot of things to do, I will continue later this weekend

Thanks! I know it is very tedious ...

I tried to fix the static code analysis warnings, but I think that they are false positives.

shrit

I left a couple of comments, and some of them are relevant.
Everything looks good to me.

shrit · 2022-09-09T09:49:50Z

src/mlpack/tests/termination_policy_test.cpp

@@ -51,7 +50,7 @@ TEST_CASE("AMFMaxIterationTerminationTest", "[TerminationPolicyTest]")
  // Custom tighter tolerance.
  MaxIterationTermination mit(10); // Only 10 iterations.
  AMF<MaxIterationTermination,
-      RandomInitialization,
+      RandomAMFInitialization,


well noticed

src/mlpack/tests/random_test.cpp

shrit · 2022-09-09T09:55:10Z

src/mlpack/tests/mock_categorical_data.hpp

@@ -24,31 +24,30 @@ inline void MockCategoricalData(arma::mat& d,
  // We'll build a spiral dataset plus two noisy categorical features.  We need
  // to build the distributions for the categorical features (they'll be
  // discrete distributions).
-  mlpack::distribution::DiscreteDistribution c1[5];
+  mlpack::DiscreteDistribution c1[5];


Are you keeping the mlpack namespace here? why?

The MockCategoricalData() function isn't in any namespace, so I fully qualify mlpack:: here. 👍

src/mlpack/tests/math_test.cpp

shrit · 2022-09-09T10:01:28Z

src/mlpack/tests/kernel_pca_test.cpp

@@ -16,9 +16,6 @@
 #include "test_catch_tools.hpp"

 using namespace mlpack;
-using namespace mlpack::math;
-using namespace mlpack::kpca;
-using namespace mlpack::kernel;
 using namespace std;


Suggested change

using namespace std;

shrit · 2022-09-09T19:37:25Z

src/mlpack/methods/naive_bayes/naive_bayes.hpp

+ *
+ * mlpack is free software; you may redistribute it and/or modify it under the
+ * terms of the 3-clause BSD license.  You should have received a copy of the
+ * 3-clause BSD license along with mlpack.  If not, see
+ * http://www.opensource.org/licenses/BSD-3-Clause for more information.


Funny how many files did not have the license in them.

Yeah, I used to have a script I ran to check that every file had a license, but it has been many years since I ran it...

shrit · 2022-09-09T19:58:06Z

src/mlpack/methods/kde/kde_main.cpp

    if (modeStr == "dual-tree")
-      kde->Mode() = KDEMode::DUAL_TREE_MODE;
+      kde->Mode() = KDEMode::KDE_DUAL_TREE_MODE;
    else if (modeStr == "single-tree")
-      kde->Mode() = KDEMode::SINGLE_TREE_MODE;
+      kde->Mode() = KDEMode::KDE_SINGLE_TREE_MODE;
  }


Is the KDE necessary here in the KDE_DUAL_TREE_MODE?

Yeah---the DUAL_TREE_MODE name is already taken by the NSMode enum (which is used for various neighbor searching classes). This might not be the best possible solution, but I think it'll work for now.

src/mlpack/methods/ann/layer/not_adapted/multiply_merge_impl.hpp

src/mlpack/methods/ann/init_rules/glorot_init.hpp

shrit · 2022-09-10T19:09:40Z

src/mlpack/core/tree/build_tree.hpp

+#ifndef MLPACK_CORE_TREE_BUILD_TREE_HPP
+#define MLPACK_CORE_TREE_BUILD_TREE_HPP
+
+#include <mlpack/prereqs.hpp>
+
+namespace mlpack {
+
+//! Construct tree that rearranges the dataset.
+template<typename TreeType, typename MatType>
+TreeType* BuildTree(
+    MatType&& dataset,
+    std::vector<size_t>& oldFromNew,
+    const typename std::enable_if<
+        TreeTraits<TreeType>::RearrangesDataset>::type* = 0)
+{
+  return new TreeType(std::forward<MatType>(dataset), oldFromNew);
+}
+
+//! Construct tree that doesn't rearrange the dataset.
+template<typename TreeType, typename MatType>
+TreeType* BuildTree(
+    MatType&& dataset,
+    const std::vector<size_t>& /* oldFromNew */,
+    const typename std::enable_if<
+        !TreeTraits<TreeType>::RearrangesDataset>::type* = 0)
+{
+  return new TreeType(std::forward<MatType>(dataset));
+}
+
+} // namespace mlpack


Now, I see where this has been moved

Co-authored-by: Omar Shrit <omar@shrit.me>

shrit

LGTM

mlpack-bot

Second approval provided automatically after 24 hours. 👍

conradsnicta · 2022-09-19T05:10:40Z

@rcurtin Seems that #3270 also has many commits that remove namespaces. This raises the risk of possible clashes. Or is #3270 meant as a followup to this PR ?

rcurtin · 2022-09-19T20:41:04Z

@rcurtin Seems that #3270 also has many commits that remove namespaces. This raises the risk of possible clashes. Or is #3270 meant as a followup to this PR ?

My error---that's meant to be merged after this one. I updated the description there.

rcurtin added 30 commits September 4, 2022 12:21

Add BSD license to all files that don't have it.

6ccd179

Remove mlpack::math:: namespace.

092f99d

Minor fixes for removing math:: namespace.

f05d0c6

Remove cv:: and hpt:: namespaces.

8340000

Remove ::hpt and ::cv namespaces from documentation.

8a73969

Remove missed mlpack::cv:: namespace bit.

18f9332

Remove mlpack::distribution:: namespace.

f33ff02

Remove uses of mlpack::distribution from tests and bindings.

352834c

Remove mlpack::kernel:: namespace.

54f9df5

Remove metric:: namespace.

6cea9e1

Remove metric namespace from bindings.

ba30784

Remove mlpack::bound:: namespace.

8031edb

First pass at removing mlpack::tree:: namespace.

1b06015

Fix some namespace resolution issues.

d380f8b

Fix ambiguities from removing addr:: namespace.

0311ace

Remove ::sfinae namespace.

874d04c

Remove tree:: namespace references from documentation.

84ae4b2

Remove mlpack::adaboost namespace.

0837952

Remove mlpack::amf:: namespace.

4ca09a4

Remove a couple dangling amf namespaces.

7b3273c

Remove regression:: namespace.

12228dc

Remove extra references to regression::.

67f3cea

Remove mlpack::neighbor:: namespace.

dd93455

Remove dbscan:: namespace.

a0b1234

Remove svd:: namespace.

1897e28

Remove cf:: namespace.

5e99533

Remove det:: namespace.

1f168d2

Remove emst:: namespace.

d3dbbd0

Remove fastmks:: namespace.

b9f62f3

Remove gmm:: namespace.

97a5fd6

rcurtin added 6 commits September 7, 2022 21:17

Remove mlpack::rl:: namespace.

fd1073b

Namespace removal fixes.

eabc449

Merge remote-tracking branch 'origin/master' into remove-namespaces

d7199d9

Add stub for namespace reverse-compatibility.

9030a05

Fixes for namespace collisions.

74e0b4b

Add the rest of the namespaces.

2b615f7

rcurtin added c: core c: methods t: added feature labels Sep 9, 2022

Add missing build_tree.hpp file.

e5929ae

Attempt to address potential static code analysis issues.

c8a5bee

rcurtin mentioned this pull request Sep 9, 2022

Discussion: everything into one namespace for mlpack 4? #3261

Closed

shrit requested changes Sep 11, 2022

View reviewed changes

rcurtin and others added 4 commits September 13, 2022 22:23

Update src/mlpack/tests/random_test.cpp

47e541f

Co-authored-by: Omar Shrit <omar@shrit.me>

Update src/mlpack/tests/math_test.cpp

8723b4a

Co-authored-by: Omar Shrit <omar@shrit.me>

Update src/mlpack/methods/ann/layer/not_adapted/multiply_merge_impl.hpp

c6465cb

Co-authored-by: Omar Shrit <omar@shrit.me>

Update src/mlpack/methods/ann/init_rules/glorot_init.hpp

b2624c0

Co-authored-by: Omar Shrit <omar@shrit.me>

rcurtin mentioned this pull request Sep 15, 2022

Compile error when #include <mlpack/methods/reinforcement_learning/q_learning.hpp> #3274

Closed

shrit approved these changes Sep 16, 2022

View reviewed changes

mlpack-bot bot approved these changes Sep 17, 2022

View reviewed changes

This was referenced Sep 19, 2022

Remove RangeType change comment #3270

Closed

MLPACK examples #2819

Closed

conradsnicta merged commit e6ad5f7 into mlpack:master Sep 21, 2022

rcurtin mentioned this pull request Sep 22, 2022

Actually install namespace_compat.hpp #3279

Merged

rcurtin mentioned this pull request Oct 14, 2022

Adapt notebooks to mlpack 4 API mlpack/examples#208

Merged

rcurtin mentioned this pull request Nov 24, 2023

Proposal for New Supervised Learning Data Simulation Classes in C++ for MLPACK Library #3559

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove namespaces #3269

Remove namespaces #3269

rcurtin commented Sep 9, 2022 •

edited

shrit commented Sep 9, 2022

rcurtin commented Sep 9, 2022

shrit left a comment

shrit Sep 9, 2022

shrit Sep 9, 2022

rcurtin Sep 14, 2022

shrit Sep 9, 2022

shrit Sep 9, 2022

rcurtin Sep 14, 2022

shrit Sep 9, 2022

rcurtin Sep 14, 2022

shrit Sep 10, 2022

shrit left a comment

mlpack-bot bot left a comment

conradsnicta commented Sep 19, 2022

rcurtin commented Sep 19, 2022

Remove namespaces #3269

Remove namespaces #3269

Conversation

rcurtin commented Sep 9, 2022 • edited

shrit commented Sep 9, 2022

rcurtin commented Sep 9, 2022

shrit 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrit left a comment

Choose a reason for hiding this comment

mlpack-bot bot left a comment

Choose a reason for hiding this comment

conradsnicta commented Sep 19, 2022

rcurtin commented Sep 19, 2022

rcurtin commented Sep 9, 2022 •

edited