Skip to content
This repository was archived by the owner on Aug 2, 2023. It is now read-only.

Add WriteCsv plus unit tests. - #2947

Merged
eerhardt merged 13 commits into
dotnet:masterfrom
dcostea:writecsv
Oct 1, 2020
Merged

eerhardt merged 13 commits into
dotnet:masterfrom
dcostea:writecsv

Conversation

@dcostea

@dcostea dcostea commented Jul 29, 2020

Copy link
Copy Markdown
Contributor

I added WriteCsv method (plus some unit tests) to write a DataFrame to CSV file. This solves #2917.

@eerhardt @pgovind I have recreated the PR following Eric's recommendations from #2945. Please review.

@pgovind pgovind left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Apologies for the late review. I left some comments now :)

@dcostea

dcostea commented Sep 10, 2020

Copy link
Copy Markdown
Contributor Author

Apologies for the late review. I left some comments now :)

Do not worry. I'm sure you guys are very busy with .NET 5 :)

var record = new StringBuilder();

bool firstRow = true;
foreach (var cell in row)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(minor perf question/suggestion): I wonder if we used the IDataView API here we could avoid the boxing and unboxing of every primitive value in the DataFrame.

For example, for each column, we can get a Getter and use it to get the value generically and write it to the StringBuilder.

See https://github.com/dotnet/machinelearning/blob/5370692f6d1e6f1f686596d530032b4226dc9e80/src/Microsoft.ML.Data/DataDebuggerPreview.cs#L44-L60 for a general structure on how to do this.

I think what we have here is good for a first implementation to get the functionality added. But we may want to benchmark alternatives to see how well this performs.

@eerhardt eerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is pretty close. After a couple minor nits I think we can merge this.

dcostea and others added 4 commits October 1, 2020 21:21
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
dcostea and others added 4 commits October 1, 2020 21:22
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

@pgovind pgovind left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just the 1 comment to rename a var. Everything else looks really good! Great work!

@dcostea

dcostea commented Oct 1, 2020

Copy link
Copy Markdown
Contributor Author

Just the 1 comment to rename a var. Everything else looks really good! Great work!

Thank you everyone for all the help! :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants