search

ben-strasser / fast-cpp-csv-parser

fast-cpp-csv-parser
BSD 3-Clause "New" or "Revised" License
2.12k stars 439 forks source link

Could I change the column_count from compile time to runtime? #26

Closed stereomatchingkiss closed 7 years ago

stereomatchingkiss commented 8 years ago

Rather than

io::CSVReader<3> in("ram.csv");

change it to

io::CSVReader<> in(3, "ram.csv");

I did the change already, it work as expected.

ben-strasser commented 8 years ago

Hi,

thanks for the feedback, however I do not see the benefits of this change.

Problems I see with the change:

Advantages I see:

Best Regards Ben Strasser

stereomatchingkiss commented 8 years ago

Thanks for you reply and the good library. In that case, I will copy the file to the project(mlpack), keep your license and do some change on it, is this Ok?

The api of mlpack need to determine the col number of the file at run time.

2016-07-23 14:55 GMT+08:00 ben-strasser notifications@github.com:

Hi,

thanks for the feedback, however I do not see the benefits of this change.

Problems I see with the change:

  • It turns a compile-time constant into a runtime variable. This prevents the optimizer from unrolling loops.
  • It introduces an interface breaking change.
  • The number of parameters one passes to read_row is inherently a constant. Why not tell the compiler this and make the number a compile-time constant?

Advantages I see:

  • Not really any, sorry

Best Regards Ben Strasser

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ben-strasser/fast-cpp-csv-parser/issues/26#issuecomment-234703425, or mute the thread https://github.com/notifications/unsubscribe-auth/ABt-uhSu8rvFYyAR3FBFj04-crbJ6THgks5qYbrGgaJpZM4JTTI2 .

ben-strasser commented 8 years ago

Hi,

In that case, I will copy the file to the project(mlpack), keep your license and do some change on it, is this Ok? Yes that would be ok.

The api of mlpack need to determine the col number of the file at run time. I'm curious about what your specific usecase is. When searching for mlpack all I can find is http://www.mlpack.org and I do not know how that library interacts with a CSV-parser.

Best Regards Ben Strasser

stereomatchingkiss commented 8 years ago

The api : 

template<typename eT>
bool Load(const std::string& filename,
          arma::Mat<eT>& matrix,
          const bool fatal = false,
const bool transpose = true);

As you can see, the api have no information about the columns of the matrix at compile time, they find them out at runtime.

The header file