Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[GSOC]DatasetMapper & Imputer #694

Merged
merged 47 commits into from Jul 25, 2016
Merged

[GSOC]DatasetMapper & Imputer #694

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
87c05a5
concept work for imputer
keon Jun 1, 2016
2e4b1a8
Merge branch 'master' of github.com:keonkim/mlpack into imputer
keon Jun 6, 2016
631e59e
do not to use NaN by default, let the user specify
keon Jun 6, 2016
391006e
Merge branch 'master' of github.com:keonkim/mlpack into imputer
keon Jun 6, 2016
6a1fb81
add template to datasetinfo and add imputer class
keon Jun 12, 2016
b0c5224
clean datasetinfo class and rename files
keon Jun 13, 2016
de35241
implement basic imputation strategies
keon Jun 13, 2016
2d38604
modify imputer_main and clean logs
keon Jun 13, 2016
bb045b8
add parameter verification for imputer_main
keon Jun 13, 2016
1295f4b
add custom strategy to impute_main
keon Jun 13, 2016
5a517c2
add datatype change in IncrementPolicy
keon Jun 14, 2016
94b7a5c
update types used in datasetinfo
keon Jun 14, 2016
ebed68f
initialize imputer with parameters
keon Jun 14, 2016
db78f39
remove datatype in dataset_info
keon Jun 15, 2016
7c60b97
Merge branch 'master' of github.com:keonkim/mlpack into imputer
keon Jun 15, 2016
da4e409
add test for imputer
keon Jun 15, 2016
d8618ec
restructure, add listwise deletion & imputer tests
keon Jun 18, 2016
3b8ffd0
fix transpose problem
keon Jun 27, 2016
90a5cd2
Merge pull request #7 from mlpack/master
keon Jun 27, 2016
32c8a73
merge
keon Jun 27, 2016
e09d9bc
updates and fixes on imputation methods
keon Jun 28, 2016
87d8d46
update data::load to accept different mappertypes
keon Jul 1, 2016
de0b2db
update data::load to accept different policies
keon Jul 1, 2016
bc187ca
add imputer doc
keon Jul 1, 2016
a340f69
debug median imputation and listwise deletion
keon Jul 2, 2016
21d94c0
remove duplicate code in load function
keon Jul 2, 2016
a92afaa
delete load overload
keon Jul 3, 2016
bace8b2
modify MapToNumerical to work with MissingPolicy
keon Jul 4, 2016
896a018
MissingPolicy uses NaN instead of numbers
keon Jul 4, 2016
1a908c2
fix reference issue in DatasetMapper
keon Jul 4, 2016
2edbc40
Move MapToNumerical(MapTokens) to Policy class
keon Jul 5, 2016
d881cb7
make policy and imputation api more consistent
keon Jul 5, 2016
a881831
numerical values can be set as missing values
keon Jul 6, 2016
63268a3
add comments and use more proper names
keon Jul 7, 2016
2eb6754
modify custom impute interface and rename variables
keon Jul 10, 2016
6d43aa3
add input-only overloads to imputation methods
keon Jul 10, 2016
fedc5e0
update median imputation to exclude missing values
keon Jul 11, 2016
787fd82
optimize imputation methods with output overloads
keon Jul 18, 2016
a0b7d59
expressive comments in imputation_test
keon Jul 18, 2016
9a6dce7
shorten imputation tests
keon Jul 18, 2016
c3aeba1
optimize preprocess imputer executable
keon Jul 18, 2016
028c217
fix bugs in imputation test
keon Jul 18, 2016
03e19a4
add more comments and delete impute_test.csv
keon Jul 22, 2016
ef4536b
Merge pull request #8 from mlpack/master
keon Jul 22, 2016
6e2c1ff
Merge branch 'master' of github.com:keonkim/mlpack into imputer
keon Jul 22, 2016
5eb9abd
fix PARAM statements in imputer
keon Jul 22, 2016
d043235
delete Impute() overloads that produce output matrix
keon Jul 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/mlpack/core/data/dataset_info.hpp
View file
Open in desktop
Expand Up @@ -113,6 +113,9 @@ class DatasetMapper
//! Modify the policy of the mapper (be careful!).
PolicyType& Policy();

//! Modify (Replace) the policy of the mapper with a new policy
void Policy(PolicyType& policy);

private:
//! Types of each dimension.
std::vector<Datatype> types;
Expand Down
9 changes: 7 additions & 2 deletions src/mlpack/core/data/dataset_info_impl.hpp
View file
Open in desktop
Expand Up @@ -18,7 +18,6 @@ template<typename PolicyType>
inline DatasetMapper<PolicyType>::DatasetMapper(const size_t dimensionality) :
types(dimensionality, Datatype::numeric)
{
Log::Debug << "DatasetMapper(dimensionality)" << std::endl;
// Nothing to initialize here.
}

Expand All @@ -28,7 +27,6 @@ inline DatasetMapper<PolicyType>::DatasetMapper(PolicyType& policy,
types(dimensionality, Datatype::numeric),
policy(std::move(policy))
{
Log::Debug << "DatasetMapper(policy, dimensionality)" << std::endl;
// Nothing to initialize here.
}

Expand Down Expand Up @@ -127,6 +125,13 @@ inline PolicyType& DatasetMapper<PolicyType>::Policy()
return this->policy;
}

template<typename PolicyType>
inline void DatasetMapper<PolicyType>::Policy(PolicyType& policy)
{
this->policy = std::move(policy);
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass by reference do not tell the user you want to move the value policy, this may give user a surprise like "oops, I do not meant to change the value of policy, but after calling it, the data of my parameter loss". To make the api more self-explain, we could

1 : pass by value

void DatasetMapper<PolicyType>::Policy(PolicyType policy)

2 : pass by universal reference

inline void DatasetMapper<PolicyType>::Policy(PolicyType&& policy)
{
    policy = std::forward<PolicyType>(policy);
}

Q : What is std::forward?
Ans : This function will convert the parameter with universal reference type to rvalue if the parameter is rvalue, to lvalue if the parameter is lvalue.

Q : Why pass by value
A : Because this would not have any side effect on the parameter(in most case) you pass in, this make codes easier to manage, user will never have a surprise like "oops, my parameter changed by the function"

Q : Why pass by universal reference
A : Because this make the api self-explain and more flexible at the same time.With this api, users have more choices, example

MissingPolicy mp;
//do something......

//do not want to move
mapper.Policy(mp)
//want to move
mapper.Policy(std::move(mp));
//or construct the policy directly, this will move too
mapper.Policy(MissingPolicy());

Copy link
Member Author

@keon keon Jul 4, 2016 •
edited

Choose a reason for hiding this comment

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

thanks for the resource, I now understand the concepts.

}



} // namespace data
} // namespace mlpack
Expand Down
33 changes: 8 additions & 25 deletions src/mlpack/core/data/load_impl.hpp
View file
Open in desktop
Expand Up @@ -65,34 +65,17 @@ void MapToNumerical(const std::vector<std::string>& tokens,
DatasetMapper<PolicyType>& info,
arma::Mat<eT>& matrix)
{
auto notNumber = [](const std::string& str)
std::stringstream token;
for (size_t i = 0; i != tokens.size(); ++i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice idea, thanks for the enhancement 👍

{
eT val(0);
std::stringstream token;
token.str(str);
token>>val;
return token.fail();
};

const bool notNumeric = std::any_of(std::begin(tokens),
std::end(tokens), notNumber);
if(notNumeric)
{
for(size_t i = 0; i != tokens.size(); ++i)
token.str(tokens[i]);
token>>matrix.at(row, i);
if (token.fail()) // if not number, map it to datasetmapper
{
const eT val = static_cast<eT>(info.MapString(tokens[i], row));
matrix.at(row, i) = val;
}
}
else
{
std::stringstream token;
for(size_t i = 0; i != tokens.size(); ++i)
{
token.str(tokens[i]);
token>>matrix.at(row, i);
token.clear();
}
token.clear();
}
}

Expand Down Expand Up @@ -411,7 +394,7 @@ bool Load(const std::string& filename,
type = "raw ASCII-formatted data";

Log::Info << "Loading '" << filename << "' as " << type << ". "
<< std::endl;
<< std::flush;
std::string separators;
if (commas)
separators = ",";
Expand Down Expand Up @@ -496,7 +479,7 @@ bool Load(const std::string& filename,
else if (extension == "arff")
{
Log::Info << "Loading '" << filename << "' as ARFF dataset. "
<< std::endl;
<< std::flush;
try
{
LoadARFF(filename, matrix, info);
Expand Down
5 changes: 1 addition & 4 deletions src/mlpack/core/data/map_policies/missing_policy.hpp
View file
Open in desktop
Expand Up @@ -29,13 +29,12 @@ class MissingPolicy

MissingPolicy()
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks quite weird, looks like my suggestion confuse you, sorry about that. Could you show me some examples, explain what kind of effects you want to achieve by MissingPolicy?Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Would be great if you could add some comments about what the function does and the parameter used.

Copy link
Member

Choose a reason for hiding this comment

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

Would be great if you could use the doxygen commands like @param for parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

{
Log::Debug << "MissingPolicy()" << std::endl;
// Nothing to initialize here.
}

explicit MissingPolicy(std::set<std::string> missingSet) :
missingSet(std::move(missingSet))
Copy link
Member

Choose a reason for hiding this comment

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

This is picky, but this line should be indented two tabs instead of just one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

{
Log::Debug << "MissingPolicy(missingSet)" << std::endl;
// Nothing to initialize here.
}

Expand All @@ -49,7 +48,6 @@ class MissingPolicy
// If this condition is true, either we have no mapping for the given string
// or we have no mappings for the given dimension at all. In either case,
// we create a mapping.
Log::Debug << "missingSet has: " << missingSet.count(string) << std::endl;
if (missingSet.count(string) != 0 &&
Copy link
Member

Choose a reason for hiding this comment

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

We could avoid this check if you added everything from missingSet to maps[dimension] in the constructor. That might give some noticeable speedup (I am not sure about that).

Copy link
Contributor

Choose a reason for hiding this comment

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

The role of missingSet and maps are different.
missingSet specify which value/string we should map to
maps record the value/string we already mapped to

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean. My thinking was, everything in missingSet is something we might expect to be in maps by the end of loading. So you can save a little bit of time by just putting everything in missingSet into maps pre-emptively. The code as it is written here only adds things into maps from missingSet when it is seen, but I am not sure it is important to have a different, standalone missingSet. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Problem is maps have different dimension need to handle, but missingSet apply to all of the dimensions. Studying Fast C++ CSV Parser, the codes make me very appreciate the expression power and performance spirit give us.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't think of that. I agree now, putting everything from missingSet into maps is not the right thing.

I agree, too, Boost spirit seems really cool, I need to play with the code in your PR to see if we can preserve decent compile times.

(maps.count(dimension) == 0 ||
maps[dimension].first.left.count(string) == 0))
Expand All @@ -64,7 +62,6 @@ class MissingPolicy
else
{
// This string already exists in the mapping.
Log::Debug << "string already exists in the mapping" << std::endl;
return maps[dimension].first.left.at(string);
}
}
Expand Down
5 changes: 0 additions & 5 deletions src/mlpack/methods/preprocess/preprocess_imputer_main.cpp
View file
Open in desktop
Expand Up @@ -92,10 +92,8 @@ int main(int argc, char** argv)
// Policy tells how the DatasetMapper should map the values.
std::set<std::string> missingSet;
missingSet.insert(missingValue);
Log::Debug << "initalize MissingPolicy(missingSet)" << endl;
MissingPolicy policy(missingSet);
using MapperType = DatasetMapper<MissingPolicy>;
Log::Debug << "initalize info(policy)" << endl;
DatasetMapper<MissingPolicy> info(policy);

Load(inputFile, input, info, true, true);
Expand Down Expand Up @@ -149,9 +147,6 @@ int main(int argc, char** argv)
}
}

// for testing purpose
Log::Info << "output::" << endl;
Log::Info << output << endl;

if (!outputFile.empty())
{
Expand Down
16 changes: 9 additions & 7 deletions src/mlpack/tests/imputation_test.cpp
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@ BOOST_AUTO_TEST_CASE(DatasetMapperImputerTest)
fstream f;
f.open("test_file.csv", fstream::out);
f << "a, 2, 3" << endl;
f << "5, 6, 7" << endl;
f << "5, 6, b" << endl;
f << "8, 9, 10" << endl;
f.close();

Expand All @@ -43,6 +43,7 @@ BOOST_AUTO_TEST_CASE(DatasetMapperImputerTest)

std::set<string> mset;
mset.insert("a");
mset.insert("b");
MissingPolicy miss(mset);
DatasetMapper<MissingPolicy> info(miss);
BOOST_REQUIRE(data::Load("test_file.csv", input, info) == true);
Expand All @@ -56,15 +57,16 @@ BOOST_AUTO_TEST_CASE(DatasetMapperImputerTest)
DatasetMapper<MissingPolicy>,
CustomImputation<double>> imputer(info);
imputer.Impute(input, output, "a", 99, dimension); // convert a -> 99
imputer.Impute(input, output, "b", 99, dimension); // convert b -> 99

BOOST_REQUIRE_CLOSE(output(0, 0), 99.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(0, 1), 2.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(0, 2), 3.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(1, 0), 5.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(0, 1), 5.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(0, 2), 8.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(1, 0), 2.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(1, 1), 6.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(1, 2), 7.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(2, 0), 8.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(2, 1), 9.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(1, 2), 9.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(2, 0), 3.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(2, 1), 99.0, 1e-5);
BOOST_REQUIRE_CLOSE(output(2, 2), 10.0, 1e-5);

// Remove the file.
Copy link
Contributor

Choose a reason for hiding this comment

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

This haven't complete?

Copy link
Member Author

Choose a reason for hiding this comment

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

yea, this is the part I wanted to test the whole process of Loading, mapping, and imputation.
I will at least make a mock data and test only mapping and imputation for this.

Expand Down