-
-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement C++ SolutionArray::save for CSV output #1508
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1508 +/- ##
==========================================
+ Coverage 70.42% 70.46% +0.03%
==========================================
Files 375 375
Lines 58285 58401 +116
Branches 20820 20897 +77
==========================================
+ Hits 41050 41152 +102
- Misses 14224 14229 +5
- Partials 3011 3020 +9
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ed7c663
to
435ddd3
Compare
Briefly gave a shot to implement the CSV parsing portion, see comment here Cantera/enhancements#163 (comment) ... I decided that this goes beyond the scope of this PR, so this is ready for a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ingmar! A big task, done well. Just a few small suggestions
Add support for data types other than float.
ee5663a
to
62290dd
Compare
Resolve discrepancies of nomenclature in C++/Python
Argument names of SolutionArray IO in C++ and Python were not consistent, which is resolved in this commit.
Thanks for the suggestions, @bryanwweber! I rebased and addressed the suggestions. I also took this occasion to improve some of the docstrings that had been somewhat neglected. In this process, I noticed some discrepancies of nomenclature between C++ and Python methods - the former used the argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ischoegl !
Thanks, @bryanwweber ... sorry for dismissing your review. Looking over the diff, I concluded that I may as well take care of documentation updates for the associated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem 😁
@bryanwweber ... again 😖. I really should use VSCode to review changes rather than pushing right away ... |
The failing samples are fixed elsewhere? |
No. These were new - and are now fixed 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thanks @bryanwweber! Will merge later tonight, unless there are more comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ischoegl. This all looks good to me.
Changes proposed in this pull request
read_csv
, wherepandas
is used by default (pandas reads CSV correctly); the fallbacknp.genfromtxt
does not handle escaped CSV entries.SolutionArray.write_csv
andSim1D.write_csv
in favor of C++ backedsave
methods.SolutionArray::restore
for CSV should be addressed by a separate PR (not necessarily before Cantera 3.0).If applicable, fill in the issue number this pull request is fixing
Addresses Cantera/enhancements#163
Closes #1372 (as long as
pandas
is installed`)If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build
&scons test
) and unit tests address code coverage