added check_input_matrices option to python bindings #2787

NippunSharma · 2020-12-28T10:23:36Z

This is in reference to #2679 .
[updated after making option more general]
I have added no_sanity_checks option for the python bindings in this PR. This option checks for nan values inside all the parameters that are of the type arma::mat , arma::colvec and arma::rowvec and throws an error if there is a nan value along with the name of the parameter.
I am working on adding this option inside other bindings as well for which I will open separate PR's.
Since this is my first PR on the automatic binding system please forgive any silly mistakes.

NippunSharma · 2020-12-28T12:39:15Z

Currently the tests are failing because different methods have different identifiers for the training / reference matrix.
Somewhere it is training and somewhere else it is input. To solve this issue I am thinking of a more generalized approach where we check for Nan values inside all parameters that are of the type arma::mat, arma::rowec or arma::colvec and report the name of the parameter that has a Nan value.

rcurtin

Hey @NippunSharma, looking good! Thank you for taking the time to work on this. I have a handful of comments; hopefully they are helpful. 👍 I think once we get this worked out, it will be easy to add this functionality to the other languages too.

rcurtin · 2020-12-31T03:20:36Z

src/mlpack/bindings/python/mlpack/io_util.hpp

+  {
+    std::string paramName = itr->first;
+    std::string paramType = itr->second.cppType;
+    if (paramType == "arma::mat")


Don't forget about arma::Mat<size_t>, arma::Row<size_t>, arma::Col<size_t>, and then the 'categorical matrix' type which is either std::tuple<data::DatasetInfo, arma::mat> or std::tuple<arma::mat, data::DatasetInfo> (I can't remember which---you can check src/mlpack/core/util/param.hpp to see how it is defined in PARAM_MATRIX_WITH_INFO(). 👍)

rcurtin · 2020-12-31T03:26:22Z

src/mlpack/bindings/python/mlpack/io_util.hpp

+/**
+ * Sanity Check.
+ */
+void SanityChecks()


What would you think about moving this function into the IO class, so that it can be called from any binding, not just the Python bindings?

I think it will be better, I have moved it

rcurtin · 2020-12-31T03:28:42Z

src/mlpack/core/util/mlpack_main.hpp

@@ -230,6 +230,8 @@ PARAM_FLAG("copy_all_inputs", "If specified, all input parameters will be deep"
    " copied before the method is run.  This is useful for debugging problems "
    "where the input parameters are being modified by the algorithm, but can "
    "slow down the code.", "");
+PARAM_FLAG("no_sanity_checks", "If specified, the input matrix is checked for"
+    " nan values.", "");


Suggested change

" nan values.", "");

" NaN values; an exception will be thrown if any are found.", "");

rcurtin · 2020-12-31T03:30:11Z

src/mlpack/core/util/mlpack_main.hpp

@@ -230,6 +230,8 @@ PARAM_FLAG("copy_all_inputs", "If specified, all input parameters will be deep"
    " copied before the method is run.  This is useful for debugging problems "
    "where the input parameters are being modified by the algorithm, but can "
    "slow down the code.", "");
+PARAM_FLAG("no_sanity_checks", "If specified, the input matrix is checked for"


Why no_sanity_checks? In this case, if we specify the flag, then we are doing the sanity checks. I might suggest a name like check_input_matrices or something like that.

You are right, I have corrected it now

rcurtin · 2020-12-31T03:30:59Z

src/mlpack/bindings/python/print_pyx.cpp

@@ -224,6 +235,10 @@ void PrintPYX(const util::BindingDetails& doc,
    cout << "  IO.SetPassed(<const string> '" << d.name << "')" << endl;
  }

+  // Before calling mlpackMain(), we do a sanity check if needed.
+  cout << "  if not IO.GetParam[cbool](<const string> 'no_sanity_checks'):" << endl;
+  cout << "    SanityChecks()" << endl;


The option is never used elsewhere in the program, so if you wanted, you could only add a block down here where you call SanityChecks(), and then no need to call IO.SetPassed() or SetParam[] or anything.

I do not fully understand what you mean here ?

You've added two blocks of code to print_pyx.cpp: the first checks the type of the option and adds it to IO via IO.SetPassed(...). The second checks the value of the parameter via IO.GetParam() and then calls SanityChecks(). What I'm suggesting is that there is no need for two blocks. You can do it all in one block, and there is no need to call SetPassed() or GetParam().

…_input_matrices

rcurtin · 2021-01-02T01:14:51Z

src/mlpack/bindings/python/mlpack/io_util.hpp

@@ -15,7 +15,7 @@

 #include <mlpack/core/util/io.hpp>
 #include <mlpack/core/data/dataset_mapper.hpp>
-
+#include <iostream>


Are you sure this is necessary?

Added by mistake, removed now

rcurtin · 2021-01-02T01:15:40Z

src/mlpack/core/util/io.cpp

+    }
+    else if (paramType == "std::tuple<data::DatasetInfo, arma::mat>")
+    {
+      if (std::get<1>(IO::GetParam<std::tuple<data::DatasetInfo, arma::mat>>(paramName)).has_nan())


A lot of lines in this file are longer than 80 characters---do you think you could wrap them please?

rcurtin · 2021-01-02T01:16:00Z

src/mlpack/bindings/python/py_option.hpp

-    // Only "verbose" and "copy_all_inputs" will be persistent.
-    if (identifier == "verbose" || identifier == "copy_all_inputs")
+    // Only "verbose", "copy_all_inputs" and "no_sanity_checks" will be persistent.
+    if (identifier == "verbose" || identifier == "copy_all_inputs" || identifier == "check_input_matrices")


I think this line is longer than 80 characters too.

rcurtin · 2021-01-02T01:18:20Z

src/mlpack/core/util/io.cpp

@@ -267,3 +267,50 @@ void IO::ClearSettings()
  GetSingleton().aliases = persistentAliases;
  GetSingleton().functionMap = persistentFunctions;
 }
+
+void IO::SanityChecks()


I think it would be a good idea to change this to CheckInputMatrices() to match the name of the option.

yes it would be better here, changed it to CheckInputMatrices()

rcurtin · 2021-01-02T01:18:35Z

src/mlpack/bindings/python/py_option.hpp

@@ -64,8 +64,8 @@ class PyOption
    data.required = required;
    data.input = input;
    data.loaded = false;
-    // Only "verbose" and "copy_all_inputs" will be persistent.
-    if (identifier == "verbose" || identifier == "copy_all_inputs")
+    // Only "verbose", "copy_all_inputs" and "no_sanity_checks" will be persistent.


Suggested change

// Only "verbose", "copy_all_inputs" and "no_sanity_checks" will be persistent.

// Only "verbose", "copy_all_inputs" and "check_input_matrices" will be persistent.

:)

rcurtin

Hey @NippunSharma, looking great! Do you think you could add a test for this functionality to src/mlpack/bindings/python/tests/?

rcurtin · 2021-01-12T01:02:37Z

src/mlpack/core/util/io.cpp

+    std::string errMsg = "The input " + paramName + " has NaN values.";
+    if (paramType == "arma::mat")
+    {
+      if (IO::GetParam<arma::Mat<double>>(paramName).has_nan())


We should probably check for Inf too---you could use .is_finite() for that. http://arma.sourceforge.net/docs.html#is_finite 👍

Yes it is a good idea, I have added the same. I decided to .has_inf() just for uniformity with .has_nan(). I have also added separate tests for the nan and inf values in src/mlpack/bindings/python/tests/test_python_binding.py

@NippunSharma maybe now it makes sense to split out the body of each if statement into a templated function? The bodies of each if statement are basically identical, so it would be great if we could capture that identical functionality in one standalone function.

Yes, that will reduce the repetitive code. I have added CheckInputMatrix() function that is called inside the CheckInputMatrices() function.

NippunSharma · 2021-01-24T19:30:46Z

@rcurtin you were correct. PARAM_COL() gives a arma::vec. I rechecked the type of matrix in all cases from here, all types seem to be correct now. However I still see errors while running the tests locally. The matrix_in is checked correctly for NaN or inf but there is some problem with the unsigned matrix input. I am looking into the problem and will try to correct it ASAP.

rcurtin · 2021-01-24T21:30:19Z

src/mlpack/bindings/python/tests/test_python_binding.py

+                                                   double_in=4.0,
+                                                   mat_req_in=[[1.0]],
+                                                   col_req_in=[1.0],
+                                                   umatrix_in=x,


Actually, this one might not work because there is no way to represent NaN or Inf with an arma::Mat<size_t>. So I think perhaps the checks are only necessary for double-type matrices or vectors.

NippunSharma · 2021-01-25T14:38:32Z

@rcurtin I have removed the tests for arma::Mat<size_t>, arma::Row<size_t> and arma::Col<size_t>. The tests are passing locally. One change was that paramType for matrix_and_info_in is actually given as TUPLE_TYPE and not std::tuple<data::DatasetInfo, arma::mat>. I have made that change in the code so that we check for TUPLE_TYPE

rcurtin · 2021-01-25T23:20:06Z

src/mlpack/core/util/io.cpp

+    {
+      IO::CheckInputMatrix(IO::GetParam<arma::mat>(paramName), paramName);
+    }
+    else if (paramType == "arma::Mat<size_t>")


I think now you can remove these if statements, since it's not possible to find a NaN or Inf in a matrix that has type size_t. 👍

rcurtin · 2021-01-25T23:22:25Z

src/mlpack/core/util/io.cpp

+      IO::CheckInputMatrix(
+          IO::GetParam<arma::Row<size_t>>(paramName), paramName);
+    }
+    else if (paramType == "TUPLE_TYPE")


I think we should hunt down why this is TUPLE_TYPE; ideally it should actually be std::tuple<mlpack::data::DatasetInfo, arma::mat>. I think it's because PARAM_MATRIX_AND_INFO_IN() is defined like this:

#define TUPLE_TYPE std::tuple<mlpack::data::DatasetInfo, arma::mat> #define PARAM_MATRIX_AND_INFO_IN(ID, DESC, ALIAS) \ PARAM_IN(TUPLE_TYPE, ID, DESC, ALIAS, TUPLE_TYPE(), false)

(That's in src/mlpack/core/util/param.hpp.) What happens if you inline that TUPLE_TYPE definition? Then we should hopefully get the right result here.

I am not able to fully understand what you are saying here. Should I remove the line #define TUPLE_TYPE std::tuple<mlpack::data::DatasetInfo, arma::mat> line and use std::tuple<mlpack::DatasetInfo, arma::mat> directly instead of using TUPLE_TYPE ?

Right, exactly---give that a shot, and then I think paramType will be std::tuple<mlpack::data::DatasetInfo, arma::mat>.

@rcurtin the following error is thrown locally after I make this change:
macro "PARAM_IN" passed 8 arguments, but takes just 6
I guess this is because the macro is taking the tuple type as 2 arguments instead of 1. Is this supposed to happen ? I have pushed the changes can you take a look ?

You're right... that must be why I defined TUPLE_TYPE to begin with. Perhaps we can't use the PARAM_IN() macro then. Take a look at how PARAM_MATRIX() is defined:

#define PARAM_MATRIX(ID, DESC, ALIAS, REQ, TRANS, IN) \ static mlpack::util::Option<arma::mat> \ JOIN(io_option_dummy_matrix_, __COUNTER__) \ (arma::mat(), ID, DESC, ALIAS, "arma::mat", \ REQ, IN, !TRANS, testName);

You can see that it just constructs an Option<arma::mat> object, and the fifth argument is the name of the type that will be param.cppType. So instead of using PARAM_IN to define PARAM_MATRIX_AND_INFO_IN(), do you think you could try an implementation like that of PARAM_MATRIX instead, which explicitly specifies the fifth parameter to Option<>?

NippunSharma · 2021-01-27T12:15:55Z

@rcurtin I have added PARAM_IN_WITH_NAME in param.hpp that takes an additional parameter NAME that will be the cppType. This solution seems to work correctly and tests are passing locally. Can you take a look ?

rcurtin · 2021-01-28T00:16:09Z

src/mlpack/core/util/param.hpp

+  #define PARAM_IN_WITH_NAME(T, ID, DESC, ALIAS, NAME, DEF, REQ) \
+      static mlpack::util::Option<T> \
+      JOIN(io_option_dummy_object_in_, __COUNTER__) \
+      (DEF, ID, DESC, ALIAS, NAME, REQ, true, false, testName);


Nice, this looks good! To keep the code maintainable (and minimal), can you adapt PARAM_COL(), PARAM_UCOL(), etc. to be expressed in terms of this macro?

NippunSharma · 2021-01-28T11:53:10Z

I have renamed the PARAM_IN_WITH_NAME() to PARAM_COMPLETE() and made it more general so that we can write all other macros in terms of it. The tests are passing locally.

rcurtin

Thanks @NippunSharma! Really cool that we were even able to reduce code duplication in param.hpp. I have a couple tiny comments; up to you how you want to handle them before merge. 👍

src/mlpack/core/util/io.cpp

rcurtin · 2021-01-29T01:30:43Z

src/mlpack/core/util/param.hpp

@@ -1223,51 +1258,10 @@ using DatasetInfo = DatasetMapper<IncrementPolicy, std::string>;
 * @param REQ Whether or not parameter is required (boolean value).
 */
 #ifdef __COUNTER__
-  #define PARAM_IN(T, ID, DESC, ALIAS, DEF, REQ) \
+  #define PARAM_COMPLETE(T, ID, DESC, ALIAS, NAME, REQ, IN, TRANS, DEF) \


Up to you, but honestly I think it would be okay to just give this the simpler name of PARAM. 👍

I was just afraid that PARAM would be already taken up by some other macro but since the tests are passing now I think it is not taken up and definitely better to use here.

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

mlpack-bot

Second approval provided automatically after 24 hours. 👍

zoq

Nice, this looks good too me as well.

rcurtin · 2021-02-02T02:23:32Z

Thanks @NippunSharma! 💯

added no_sanity_checks option to python bindings

266ae6e

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Dec 28, 2020

made no_sanity_checks more general

f1368e2

NippunSharma marked this pull request as ready for review December 29, 2020 10:05

NippunSharma changed the title ~~[WIP] added no_sanity_checks option to python bindings~~ added no_sanity_checks option to python bindings Dec 29, 2020

NippunSharma mentioned this pull request Dec 29, 2020

Add 'sanity check' option to bindings #2679

Closed

6 tasks

rcurtin reviewed Dec 31, 2020

View reviewed changes

NippunSharma added 5 commits December 31, 2020 20:44

added arma::Mat<size_t> and categorical data

6ef1eca

moved SanityCheck() to IO class and changed no_sanity_checks to check…

22f5932

…_input_matrices

changed comments

124d39d

wrapped function with Cython

e5bfb40

indentation removed

c3ff8d3

rcurtin reviewed Jan 2, 2021

View reviewed changes

NippunSharma added 6 commits January 2, 2021 12:48

removed iostream

ee889c7

changed name from SanityChecks() to CheckInputMatrices()

8a8f0ea

made single block in print_pyx.cpp

62c9f50

reduced num of chars per line in io.cpp

be82a0d

reduced num of chars per line in py_option.hpp

990ef53

changed function name while wrapping

7d6a209

NippunSharma requested a review from rcurtin January 3, 2021 02:50

NippunSharma changed the title ~~added no_sanity_checks option to python bindings~~ added check_input_matrices option to python bindings Jan 4, 2021

rcurtin reviewed Jan 12, 2021

View reviewed changes

added inf functionality and tests

aeb0f49

zoq removed s: unanswered s: unlabeled labels Jan 12, 2021

NippunSharma added 2 commits January 24, 2021 21:14

added check for unsigned matrix

0d31176

fixing parameter types while checking

954f3c4

rcurtin reviewed Jan 24, 2021

View reviewed changes

removing size_t tests

f5b32bc

NippunSharma requested a review from rcurtin January 25, 2021 17:06

rcurtin reviewed Jan 25, 2021

View reviewed changes

NippunSharma added 5 commits January 26, 2021 19:08

removed size_t conditions from CheckInputMatrices()

7ec4058

made definition of TUPLE_TYPE inline

16ff91b

removed TUPLE_TYPE

458b302

minor change

ab3110e

added PARAM_IN_WITH_NAME

0914fe9

rcurtin reviewed Jan 28, 2021

View reviewed changes

NippunSharma added 2 commits January 28, 2021 13:02

changed PARAM_IN_WITH_NAME to PARAM_COMPLETE and editted other macros

5b76b2f

added comments

3664422

rcurtin approved these changes Jan 29, 2021

View reviewed changes

NippunSharma and others added 2 commits January 29, 2021 18:04

Update src/mlpack/core/util/io.cpp

23c3903

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

changed name to PARAM

9a3815f

mlpack-bot bot approved these changes Jan 30, 2021

View reviewed changes

mlpack-bot bot removed the s: needs review label Jan 30, 2021

zoq approved these changes Jan 30, 2021

View reviewed changes

rcurtin merged commit 041617d into mlpack:master Feb 2, 2021

This was referenced Oct 14, 2022

Release version 4.0.0 #3285

Closed

Release version 4.0.0 #3286

Closed

rcurtin mentioned this pull request Oct 23, 2022

Release version 4.0.0 #3293

Merged

	" nan values.", "");
	" NaN values; an exception will be thrown if any are found.", "");

	// Only "verbose", "copy_all_inputs" and "no_sanity_checks" will be persistent.
	// Only "verbose", "copy_all_inputs" and "check_input_matrices" will be persistent.

added check_input_matrices option to python bindings #2787

added check_input_matrices option to python bindings #2787

Conversation

NippunSharma commented Dec 28, 2020 • edited

NippunSharma commented Dec 28, 2020

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

NippunSharma Dec 31, 2020 • edited

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

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

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

NippunSharma commented Jan 24, 2021

Choose a reason for hiding this comment

NippunSharma commented Jan 25, 2021 • edited

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

NippunSharma commented Jan 27, 2021

Choose a reason for hiding this comment

NippunSharma commented Jan 28, 2021

rcurtin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NippunSharma Jan 30, 2021 • edited

Choose a reason for hiding this comment

mlpack-bot bot left a comment

Choose a reason for hiding this comment

zoq left a comment

Choose a reason for hiding this comment

rcurtin commented Feb 2, 2021

NippunSharma commented Dec 28, 2020 •

edited

NippunSharma Dec 31, 2020 •

edited

NippunSharma commented Jan 25, 2021 •

edited

NippunSharma Jan 30, 2021 •

edited