Adding Data class for data pre-processing #2727

heisenbuug · 2020-11-22T00:42:18Z

As discussed in #2722, I implemented the data class. Describe method works only on numeric attribute types.

.info() will give output:

.describe() will give output:

rxng8

I think this looks good. The only thing I notice is that should we have one file for header and one file for header implementation?

rxng8 · 2020-11-22T16:31:50Z

Referring to #2715, I think one should also notice the user if they cannot load the data. I.e :

if (!Data::Load()):
     FAIL(<message>)

heisenbuug · 2020-11-23T05:53:38Z

I think this looks good. The only thing I notice is that should we have one file for header and one file for header implementation?

Yea, I am planning to convert it into two files, this way we can maintain consistency over styling.

rcurtin

Hey @heisenbuug, thanks for taking the initiative and implementing this! It actually looks like what you are implementing is a generic dataframe-like class... which is great! Do you think that we could use some of the existing functionality we have with data::DatasetInfo? For instance, consider adding a constructor that can take an already-constructed matrix and DatasetInfo.

Also, do you think you could add some tests and fix the style issues? We don't need to check the exact format of the output (since we may change how things are printed later), but we should at least check that the various operations work and compile correctly.

I know I am writing a lot; sorry about that. This is the beginning of some pretty important support for arbitrary datatypes, so I want to make sure we get it started while thinking about all the eventual uses we might have. :) I could see this being the precursor of a separate C++ dataframe library, for instance. Do you think we could use support in this class to replace the implenentations inside the bindings in methods/preprocess/?

src/mlpack/core/data/pre_processing.hpp

rcurtin · 2020-12-01T15:42:31Z

src/mlpack/core/data/pre_processing.hpp

+
+#include <mlpack/core.hpp>
+#include <fstream>
+#include <vector>


I think these two are already included by core.hpp. Also, if you can, try to include only mlpack/prereqs.hpp since it will include fewer other files and help keep compilation fast. 👍

src/mlpack/core/data/pre_processing.hpp

src/mlpack/core/data/pre_processing_impl.hpp

rcurtin · 2020-12-01T15:49:28Z

src/mlpack/core/data/pre_processing_impl.hpp

+  void Data:: info()
+  {
+    // Returns Index range, list of headers
+    std::cout << typeid(data).name() << std::endl;


This isn't guaranteed to be a readable description of the name---it might be tedious, but we should probably find another solution here.

src/mlpack/core/data/pre_processing_impl.hpp

rcurtin · 2020-12-01T15:51:45Z

src/mlpack/core/data/pre_processing_impl.hpp

+
+    std::cout.setf(std::ios::fixed);
+    std::cout.width(5);
+    std::cout << "Count: " << std::endl << stats.count() << std::endl;


Maybe we should prefix the values with the name of the dimension?

heisenbuug · 2020-12-01T18:01:28Z

Hello @rcurtin, thank you for the detailed look. I noticed many of the mistakes myself and I am working on them.

I know I am writing a lot; sorry about that. This is the beginning of some pretty important support for arbitrary datatypes, so I want to make sure we get it started while thinking about all the eventual uses we might have. :) I could see this being the precursor of a separate C++ dataframe library, for instance. Do you think we could use support in this class to replace the implenentations inside the bindings in methods/preprocess/?

That's what my plan is, to keep adding the functionalities once we get the basic implementation ready.
I will soon make a PR for this which will include all the changes and some other functionalities that I have added.

Also, I have some concerns regarding the implementation of certain features, if you can look at them, shall I add it to the #2722
or maybe we can use IRC for that?

rcurtin · 2020-12-01T21:35:49Z

Up to you, maybe easier to add to #2722 so that it's all in one place 👍

…future plans

rcurtin

Hey @heisenbuug, I took a quick look. Hopefully my comments are helpful. I didn't look too deeply (yet) because I still think there are some high level design questions to be addressed. 👍 More details in my comments. Thanks!

rcurtin · 2021-01-05T21:16:02Z

src/mlpack/core/data/pre_processing.hpp

+ * http://www.opensource.org/licenses/BSD-3-Clause for more information.
+ */
+#ifndef MLPACK_CORE_DATA_DATA_PRE_PROCESSING_HPP
+#define MLPACK_CORE_DATA_DATA_PRE_PROCESSING_HPP


I think you've doubled DATA in the macro name here. :)

rcurtin · 2021-01-05T21:18:46Z

src/mlpack/core/data/pre_processing.hpp

+ * @param numericAttributes Bool array to indicate numeric attributes
+ * @param numericData Arma mat of numeric attributes only
+ */
+class Data {


So my primary suggestion here is that this class is basically storing data and the set of dimensions that are categorical. But we already have basically this in the data::DatasetInfo class. How can we redo this code so that it fits (or adapts) that existing support that we already have? I don't want to end up with two very different ways of achieving the same support---that makes the library feel schizophrenic and not unified. 👍

Heyy, first of all, sorry for the late reply and not following up, okay, I will try and redo the code so that it can fit here...

mlpack-bot · 2021-02-04T21:56:53Z

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍

heisenbuug · 2021-03-01T11:21:11Z

@rcurtin, we have two ways to load data...
One that is implemented in mlpack::data::Load and other one is arma::mat.load()
We already have two implementations to do same thing, how should I implement new dataframe-like class then? Should it have it's own load method or something else, any suggestions?

rcurtin · 2021-03-06T22:35:22Z

We generally don't use arma::mat.load() inside of mlpack. All data loading is done via data::Load(), and when we are loading categorical data, we use the overload that also takes a data::DatasetInfo (which encodes whether a dimension is categorical or numeric, and any mappings that are used to represent the categories as values inside an Armadillo matrix). My suggestion would be that if we are going to add a dataframe-type class, it should take over all the functionality that's currently being provided by passing around both an arma::mat and a data::DatasetInfo.

mlpack-bot · 2021-04-05T23:11:09Z

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍

heisenbuug added 2 commits November 22, 2020 06:05

Added Data Class for data pre-processing

81c66df

New line at end

9b6421f

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Nov 22, 2020

heisenbuug changed the title ~~Data class~~ Adding Data class for data pre-processing Nov 22, 2020

rxng8 reviewed Nov 22, 2020

View reviewed changes

kartikdutt18 added c: methods t: added feature and removed s: unanswered s: unlabeled labels Nov 22, 2020

heisenbuug added 3 commits November 23, 2020 16:06

two .hpp files

25b3650

Update pre_processing.hpp

c2bd292

Update pre_processing_impl.hpp

53772aa

rcurtin reviewed Dec 1, 2020

View reviewed changes

heisenbuug added 4 commits December 7, 2020 07:50

Made some modification as suggested and added commments and a bit of …

4ffa103

…future plans

Merge branch 'DataClass' of github.com:heisenbuug/mlpack into DataClass

4dfc90b

Made some modification as suggested and added commments and a bit of …

03a6bec

…future plans

a constructor that can take an already-constructed mat and headers

bbba189

rcurtin mentioned this pull request Dec 22, 2020

Adding Data pre-processing utils to mlpack #2722

Closed

9 tasks

rcurtin reviewed Jan 5, 2021

View reviewed changes

mlpack-bot bot added s: stale and removed s: stale labels Feb 4, 2021

heisenbuug mentioned this pull request Apr 3, 2021

"The future of mlpack", round two! #2524

Closed

mlpack-bot bot added the s: stale label Apr 5, 2021

mlpack-bot bot closed this Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Data class for data pre-processing #2727

Adding Data class for data pre-processing #2727

heisenbuug commented Nov 22, 2020

rxng8 left a comment

rxng8 commented Nov 22, 2020

heisenbuug commented Nov 23, 2020

rcurtin left a comment

rcurtin Dec 1, 2020

rcurtin Dec 1, 2020

rcurtin Dec 1, 2020

heisenbuug commented Dec 1, 2020

rcurtin commented Dec 1, 2020

rcurtin left a comment

rcurtin Jan 5, 2021

rcurtin Jan 5, 2021

heisenbuug Feb 6, 2021

mlpack-bot bot commented Feb 4, 2021

heisenbuug commented Mar 1, 2021

rcurtin commented Mar 6, 2021

mlpack-bot bot commented Apr 5, 2021

Adding Data class for data pre-processing #2727

Adding Data class for data pre-processing #2727

Conversation

heisenbuug commented Nov 22, 2020

rxng8 left a comment

Choose a reason for hiding this comment

rxng8 commented Nov 22, 2020

heisenbuug commented Nov 23, 2020

rcurtin left a comment

Choose a reason for hiding this comment

rcurtin Dec 1, 2020

Choose a reason for hiding this comment

rcurtin Dec 1, 2020

Choose a reason for hiding this comment

rcurtin Dec 1, 2020

Choose a reason for hiding this comment

heisenbuug commented Dec 1, 2020

rcurtin commented Dec 1, 2020

rcurtin left a comment

Choose a reason for hiding this comment

rcurtin Jan 5, 2021

Choose a reason for hiding this comment

rcurtin Jan 5, 2021

Choose a reason for hiding this comment

heisenbuug Feb 6, 2021

Choose a reason for hiding this comment

mlpack-bot bot commented Feb 4, 2021

heisenbuug commented Mar 1, 2021

rcurtin commented Mar 6, 2021

mlpack-bot bot commented Apr 5, 2021