-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add serialization support for Client and Dataset #293
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## develop #293 +/- ##
===========================================
- Coverage 77.13% 76.48% -0.65%
===========================================
Files 66 66
Lines 4151 4266 +115
===========================================
+ Hits 3202 3263 +61
- Misses 949 1003 +54
|
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.
Looked through the *.py/pybind/Py*.cpp
files in detail and have a couple of small notes, but otherwise everything looks good. Skimmed through the reset and didn't see any immediate red flags.
Overall looks great!! This should be very helpful for users attempting to debug their simulations!!
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.
Looks great and again, a great quality-of-life improvement. Feel free to ignore the comment about passing by value. However, I would request that you change the write unit to something something other than STDERR. Those are two-line fixes and don't need to re-review. Thanks @billschereriii!
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.
LGTM!!
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.
Just one minor question/suggestion. Otherwise, great addition to the library and LGTM!
C, C++, Fortran get to_string() methods
C++ also gets ostream& operator<<() methods
Python gets str() methods
Sample output (Client):
Sample output (DataSet):