Robust dataframe io - Githubissues

dotnet / corefxlab

This repo is for experimentation and exploring new ideas that may or may not make it into the main corefx repo.

MIT License

1.46k stars 345 forks source link

Robust dataframe io #2938

Closed chriss2401 closed 4 years ago

chriss2401 commented 4 years ago

Hi @eerhardt and @pgovind ,

The following pull request adds the following features to Microsoft.Data.Analysis :

Separate M.D.A and M.D.A.I.O projects (pulled from pgovind:DataframeIO )
Removed M.D.A.I.O related duplicate code from M.D.A and moved needed code to IO project (i.e. main high level LoadCsv function).
Added a CultureInfo object as a input parameter in the public API with unit tests - related to #2926
Added better exception handling/message printing when type convertions fail with unit tests - related to #2902
- If the type that fails is single/double, a corresponding NaN is assigned.

Please review at your own convenience.

Christos.

dnfadmin commented 4 years ago

All CLA requirements met.

chriss2401 commented 4 years ago

Hi @pgovind and @eerhardt,

If there is consensus on keeping only one project (M.D.A), then I can roll back the changes and open a PR that only closes #2926, and then another one for #2902.

Another option could be that there is a high level public function in M.D.A which loads a csv and then all the helper functions are in the I.O. related one. That way there are two separations and the user can just call M.D.A.

pgovind commented 4 years ago

@chriss2401 I think rolling back the changes and fixing only #2926 and #2902 is a good idea! I really like the fixes for those 2 issues. Once my .NET 5 work is done, I'll work on a full fledged LoadCsv method that should fix all our parsing concerns with LoadCsv

chriss2401 commented 4 years ago

@chriss2401 I think rolling back the changes and fixing only #2926 and #2902 is a good idea! I really like the fixes for those 2 issues. Once my .NET 5 work is done, I'll work on a full fledged LoadCsv method that should fix all our parsing concerns with LoadCsv

Sounds good! I opened a PR for #2902 , will afterwards open one for #2926. Closing this PR :)