Add CSV data parsing #402

hidmic · 2022-07-27T14:21:07Z

🎉 New feature

Summary

This patch adds enough functionality to read a data frame from a CSV stream of data. This will come in handy for plugins that need additional data, external to the simulation.

Needs gazebosim/gz-math#456.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

chapulina · 2022-07-27T18:48:22Z

Instead of maintaining a custom CSV parser, maybe we can depend on / vendor an existing one? This is the first one that came up on my search: https:/d99kris/rapidcsv

hidmic · 2022-07-27T19:13:15Z

Instead of maintaining a custom CSV parser, maybe we can depend on / vendor an existing one?

@chapulina absolutely. There's no de-facto standard out there though, which is why I crafted a minimal implementation.

This is the first one that came up on my search: https:/d99kris/rapidcsv

That one is header-only and source distributed, so we would have to vendor it. Code's simple enough.

hidmic · 2022-07-29T14:14:31Z

@chapulina as per our discussion offline, I've updated this patch to rely on libcsv-dev for CSV parsing. I've also kept an iterator-based interface to it to avoid polluting the global namespace for gz-common. PTAL!

scpeters · 2022-07-29T16:19:55Z

include/gz/common/DataFrame.hh

+#include <gz/common/CSVStreams.hh>
+#include <gz/common/Io.hh>
+
+#include <gz/math/TimeVaryingVolumetricGrid.hh>


currently there are gz-common components that depend on gz-math, but the gz-common core library does not. We should consider moving the code in this PR to its own csv component or adding a math dependency to gz-common core

this could also be added as a component of gz-utils

We should consider moving the code in this PR to its own csv component

Moved the whole IO concept to its own component in a925186. Haven't tested the Bazel bit yet but hopefully I didn't messed it up.

traversaro · 2022-07-29T17:22:40Z

@chapulina as per our discussion offline, I've updated this patch to rely on libcsv-dev for CSV parsing. I've also kept an iterator-based interface to it to avoid polluting the global namespace for gz-common. PTAL!

Totally ok for me, but just to double check: are you aware that this would add a LGPL dependency that cannot be skipped to gz-common?

chapulina · 2022-07-29T17:58:37Z

Totally ok for me, but just to double check: are you aware that this would add a LGPL dependency that cannot be skipped to gz-common?

Thanks for bringing it up, @traversaro , we hadn't noticed that. Still iterating.

chapulina · 2022-07-30T00:06:23Z

Removed the Breaking change because the dependency on libcsv will be removed.

Signed-off-by: Michel Hidalgo <[email protected]>

This reverts commit 380845f. Signed-off-by: Michel Hidalgo <[email protected]>

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2022-08-05T11:42:44Z

gazebosim/gz-math#456 is merged. This should pass CI in some hours.

@caguero mind to give it a pass?

chapulina · 2022-08-06T01:52:47Z

Removed the Garden label because we're past feature freeze ❄️ .

Since this PR is backwards-compatible, it could be targeted at Citadel and merged forward from there.

caguero · 2022-08-08T17:18:44Z

Reminder: We'll need to update gz-common5-release with the new component. @j-rivero .

caguero · 2022-08-08T16:55:28Z

io/src/CSVStreams.cc

+
+const CSVDialect CSVDialect::Unix = {',', '\n', '"'};
+
+bool operator==(const CSVDialect &_lhs, const CSVDialect &_rhs)


Nit: Use function delimiter /////////////////////////////////////////////////. See other .cc files for examples.

caguero · 2022-08-08T16:56:46Z

io/src/CSVStreams.cc

+ }
+ }
+
+ public: std::istream *stream{nullptr};


Add doxygen to these data members.

caguero · 2022-08-08T17:06:36Z

io/include/gz/common/Io.hh

+ {
+ template <typename T>
+ struct IO {
+ static T ReadFrom(std::istringstream &_istream)


Document these functions?

caguero · 2022-08-08T17:09:26Z

io/include/gz/common/CSVStreams.hh

+ char quote;
+
+ /// CSV dialect as expected by Unix tools.
+ static const CSVDialect Unix;


Does this parser work with Windows for example? If not I think we should mention it somewhere.

@caguero I'll start by saying I didn't try this on Windows, BUT my understanding is that Windows will translate \r\n to \n when reading a text file (see here and here) so this dialect should work just fine there too.

Signed-off-by: Michel Hidalgo <[email protected]>

codecov · 2022-08-09T20:03:09Z

Codecov Report

Merging #402 (be13770) into main (81c2ebd) will increase coverage by 0.10%.
The diff coverage is 86.82%.

@@            Coverage Diff             @@
##             main     #402      +/-   ##
==========================================
+ Coverage   80.38%   80.48%   +0.10%     
==========================================
  Files          85       88       +3     
  Lines        9944    10111     +167     
==========================================
+ Hits         7993     8138     +145     
- Misses       1951     1973      +22

Impacted Files	Coverage Δ
io/include/gz/common/DataFrame.hh	`76.27% <76.27%> (ø)`
io/src/CSVStreams.cc	`92.00% <92.00%> (ø)`
io/include/gz/common/Io.hh	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

caguero

Minor documentation comments but looks good to me!

caguero · 2022-08-09T21:27:59Z

io/include/gz/common/DataFrame.hh

+ class DataFrame
+ {
+ /// \brief Check if column key is present.
+ public: bool Has(const K &_key) const


Document _key and return value.

Done in 9a3c84d.

caguero · 2022-08-09T21:28:04Z

io/include/gz/common/DataFrame.hh

+ }
+
+ /// \brief Fetch mutable reference to column
+ public: V &operator[](const K &_key)


Document _key and return value.

Done in 9a3c84d.

caguero · 2022-08-09T21:28:09Z

io/include/gz/common/DataFrame.hh

+ }
+
+ /// \brief Fetch immutable reference to column
+ public: const V &operator[](const K &_key) const


Document _key and return value.

Done in 9a3c84d.

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2022-08-10T15:16:51Z

@caguero the Homebrew PR job is failing for unrelated reasons, see here.

BTW how do we go about validating the Bazel BUILD file?

scpeters · 2022-08-10T16:56:22Z

@caguero the Homebrew PR job is failing for unrelated reasons, see here.

I believe that compiler warning has been fixed by #416

mjcarroll · 2022-08-15T21:06:03Z

BTW how do we go about validating the Bazel BUILD file?

Don't worry about it for now, I have a batch of Bazel updates coming.

hidmic · 2022-08-16T20:10:38Z

OK, I'll merge this into main, then backport to mbari/gz-common5. FYI @caguero.

Signed-off-by: Michel Hidalgo <[email protected]> Co-authored-by: Carlos Agüero <[email protected]>

hidmic added the MBARI-LRAUV Sponsored by MBARI-LRAUV project: https:/osrf/lrauv label Jul 27, 2022

github-actions bot added the 🌱 garden Ignition Garden label Jul 27, 2022

hidmic changed the title ~~Add CSV parsing support~~ Add CSV data parsing Jul 27, 2022

hidmic mentioned this pull request Jul 27, 2022

2022 M3 meta-issue osrf/lrauv#225

Closed

22 tasks

chapulina added the enhancement New feature or request label Jul 27, 2022

hidmic mentioned this pull request Jul 29, 2022

Add FindCSV.cmake module gazebosim/gz-cmake#290

Closed

8 tasks

hidmic marked this pull request as ready for review July 29, 2022 14:12

hidmic requested a review from mjcarroll as a code owner July 29, 2022 14:12

This was referenced Jul 29, 2022

CI Configuration for new dependency in #402 #407

Closed

gz-common5: depend on libcsv osrf/homebrew-simulation#1976

Merged

scpeters reviewed Jul 29, 2022

View reviewed changes

chapulina added Breaking change Breaks API, ABI or behavior. Must target unstable version. needs upstream release Blocked by a release of an upstream library labels Jul 29, 2022

chapulina removed the Breaking change Breaks API, ABI or behavior. Must target unstable version. label Jul 30, 2022

hidmic added 3 commits August 1, 2022 17:07

Add string trimming utility

cb1fc16

Signed-off-by: Michel Hidalgo <[email protected]>

Add CSV data parsing support

4d7bc1f

Signed-off-by: Michel Hidalgo <[email protected]>

Use libcsv for minimal maintenance surface

8de20ad

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic force-pushed the hidmic/csv-data branch from 809f7b6 to 7014c09 Compare August 1, 2022 20:09

hidmic added 4 commits August 1, 2022 17:42

Revert "Add string trimming utility"

19da6f8

This reverts commit 380845f. Signed-off-by: Michel Hidalgo <[email protected]>

Document and please linters

395aee1

Signed-off-by: Michel Hidalgo <[email protected]>

Drop libcsv as a dependency

150bb26

Signed-off-by: Michel Hidalgo <[email protected]>

Move CSV data parsing to IO component

a925186

Signed-off-by: Michel Hidalgo <[email protected]>

chapulina removed the 🌱 garden Ignition Garden label Aug 6, 2022

caguero reviewed Aug 8, 2022

View reviewed changes

Address peer review comments

8673ed4

Signed-off-by: Michel Hidalgo <[email protected]>

Merge branch 'main' into hidmic/csv-data

aab2ff3

caguero approved these changes Aug 9, 2022

View reviewed changes

hidmic added 3 commits August 10, 2022 08:50

Add missing documentation

9a3c84d

Signed-off-by: Michel Hidalgo <[email protected]>

Relocate visibility attributes

e72604a

Signed-off-by: Michel Hidalgo <[email protected]>

Add missing visibility attribute

f6e5a12

Signed-off-by: Michel Hidalgo <[email protected]>

Merge branch 'main' into hidmic/csv-data

4008a99

hidmic changed the base branch from main to mbari/gz-common5 August 16, 2022 20:08

hidmic changed the base branch from mbari/gz-common5 to main August 16, 2022 20:09

Merge branch 'main' into hidmic/csv-data

be13770

hidmic merged commit 4a982d7 into main Aug 17, 2022

hidmic deleted the hidmic/csv-data branch August 17, 2022 13:50

hidmic added a commit that referenced this pull request Aug 17, 2022

Add CSV data parsing (#402)

74bb454

Signed-off-by: Michel Hidalgo <[email protected]> Co-authored-by: Carlos Agüero <[email protected]>

hidmic mentioned this pull request Sep 2, 2022

Skip CSV header when reading DataFrame #435

Merged

7 tasks

hidmic added a commit that referenced this pull request Oct 17, 2022

Add CSV data parsing (#402)

23cb16b

Signed-off-by: Michel Hidalgo <[email protected]> Co-authored-by: Carlos Agüero <[email protected]>

hidmic mentioned this pull request Oct 17, 2022

[Backport] CSV data support #460

Merged

hidmic added a commit that referenced this pull request Oct 17, 2022

Add CSV data parsing (#402)

10ff92c

Signed-off-by: Michel Hidalgo <[email protected]> Co-authored-by: Carlos Agüero <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CSV data parsing #402

Add CSV data parsing #402

hidmic commented Jul 27, 2022 •

edited

Loading

chapulina commented Jul 27, 2022

hidmic commented Jul 27, 2022

hidmic commented Jul 29, 2022

scpeters Jul 29, 2022

scpeters Jul 29, 2022

hidmic Aug 1, 2022 •

edited

Loading

traversaro commented Jul 29, 2022

chapulina commented Jul 29, 2022

chapulina commented Jul 30, 2022

hidmic commented Aug 5, 2022

chapulina commented Aug 6, 2022

caguero commented Aug 8, 2022

caguero Aug 8, 2022

caguero Aug 8, 2022

caguero Aug 8, 2022

caguero Aug 8, 2022

hidmic Aug 8, 2022 •

edited

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading

caguero left a comment

caguero Aug 9, 2022

hidmic Aug 10, 2022

caguero Aug 9, 2022

hidmic Aug 10, 2022

caguero Aug 9, 2022

hidmic Aug 10, 2022

hidmic commented Aug 10, 2022

scpeters commented Aug 10, 2022

mjcarroll commented Aug 15, 2022

hidmic commented Aug 16, 2022


		const CSVDialect CSVDialect::Unix = {',', '\n', '"'};

		bool operator==(const CSVDialect &_lhs, const CSVDialect &_rhs)

Add CSV data parsing #402

Add CSV data parsing #402

Conversation

hidmic commented Jul 27, 2022 • edited Loading

🎉 New feature

Summary

Checklist

chapulina commented Jul 27, 2022

hidmic commented Jul 27, 2022

hidmic commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

traversaro commented Jul 29, 2022

chapulina commented Jul 29, 2022

chapulina commented Jul 30, 2022

hidmic commented Aug 5, 2022

chapulina commented Aug 6, 2022

caguero commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2022 • edited Loading

Codecov Report

caguero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Aug 10, 2022

scpeters commented Aug 10, 2022

mjcarroll commented Aug 15, 2022

hidmic commented Aug 16, 2022

hidmic commented Jul 27, 2022 •

edited

Loading

hidmic Aug 1, 2022 •

edited

Loading

hidmic Aug 8, 2022 •

edited

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading