akielaries / openGPMP

Hardware Accelerated General Purpose Mathematics Package
https://akielaries.github.io/openGPMP/
MIT License
8 stars 3 forks source link

Add DataTable class for issue #29 #53

Closed akielaries closed 1 year ago

akielaries commented 1 year ago

As mentioned in #29, this patch implements the early stages of the openMTPK DataTable class creating 3 specific types.

// alias for the pair type of strings
typedef std::pair<std::vector<std::string>,
                  std::vector<std::vector<std::string>>>
    DataTableStr;
// alias for pair type of 64 bit integers
typedef std::pair<std::vector<int64_t>, std::vector<std::vector<int64_t>>>
    DataTableInt;

// alias for pair type of long doubles
typedef std::pair<std::vector<long double>,
                  std::vector<std::vector<long double>>>
    DataTableDouble;

The DataTable class currently houses 4 functions 2 of which serve as the core of the class, csv_read & display.

    DataTableStr csv_read(std::string filename, std::vector<std::string> columns = {});
    template <typename T>
    void display(std::pair<std::vector<T>, std::vector<std::vector<T>>> data, bool display_all = false);

The csv_read function parses CSV files storing them as an std::pair of std::vectors of std::string type. This defaults each value to a string type therefore spawning the creation of conversion methods. display is a template method that takes in pairs of vectors of any type.

    // converts DataTableStr -> DataTableInt 
    DataTableInt str_to_int(DataTableStr src);
    // converts DataTableStr -> DataTableDouble
    DataTableDouble str_to_double(DataTableStr src);

Since the default datatype of the csv_read functions is a string, methods to convert columns containing numerical types to integers (64-bit) or doubles (varying width, 64-bit on my machine) were put in place. The display function does not retain the information of column headers meaning if columns are dropped/lost the DataTable object will not keep the column headers.

Moved struct dir to core and added /modules/core/datatable.cpp to CMakeLists.txt for installation

Fixes #29

akielaries commented 1 year ago

Tagging @sidsbrmnn for review

kgimlay commented 1 year ago

@akielaries Thank you for your contribution (to your own project). I have reviewed it and have only found one thing astray.

In modules/core/datatable.hpp/.cpp there is a member function that is implemented within the app instead of being only prototypes in the hpp and implemented in the cpp. The rest of the member function are implemented in the cpp. I would recommend moving the implementation to the cpp file.

Your contribution looks good otherwise! Have a great day!

P.S. I believe my team mates will will be adding their own comments soon to follow!

akielaries commented 1 year ago

@kgimlay With C++ template related methods I've found it easier to just leave the implementation in the header file just like with a template C++ class. Otherwise, to my knowledge, I'd have to explicitly instantiate the type of each function. I've just found it as a way to get around having to declare each type of the function at the bottom of an implementation file.

kgimlay commented 1 year ago

@akielaries this makes sense.

We looked a little more at your code and the only things we can see potential issues with are:

  1. we would love to see comments describing your code and,
  2. changing hard coded values into either constants or macros could make your code more robust.

Otherwise, this looks great!