-
-
Notifications
You must be signed in to change notification settings - Fork 348
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
Support file I/O for string columns in SolutionArray #900
Support file I/O for string columns in SolutionArray #900
Conversation
Implementation based on a suggestion by Bryan W. Weber <bryan.w.weber@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #900 +/- ##
=======================================
Coverage 71.17% 71.18%
=======================================
Files 376 376
Lines 46190 46201 +11
=======================================
+ Hits 32877 32888 +11
Misses 13313 13313
Continue to review full report at Codecov.
|
b669247
to
11677fd
Compare
11677fd
to
a0a19c1
Compare
@bryanwweber ... enabling strings wasn’t difficult after all. However, any comments (or better ideas) on the numpy version if/else condition here? (Alternative would be a less specific It’s pretty quiet these days otherwise: I hope that the work on beta is coming along. |
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. In 6edb133 we set the minimum NumPy version to 1.12, since that's required for #838 to work properly. Ubuntu 18.04 provides 1.13.x in the system packages, and we didn't want to break compatibility with that version, so that's how I picked 1.12. Anyhow, I left a comment below about the version comparison.
Too many other things going on right now, with the semester approaching. It is my goal to get 2.5.0 out before the semester (i.e., before August 31) but... you know 😄
Address review comments
@bryanwweber ... thanks for the review: your recommendations were useful as always and were easily addressed. I'm happy about the suggestion for Definitely looking forward to the release of 2.5.0 - I appreciate your and @speth's efforts on getting this out! I won't teach combustion again until next spring though 😜 PS: failing tests are fixed by #905 |
e20368f
to
2fb21e9
Compare
@bryanwweber ... thanks again for your feedback - the comment is now added; I believe everything is taken care of at this point. |
The length of strings assigned by built-in SolutionArray methods is limited to 50 characters.
2fb21e9
to
e4e7f3d
Compare
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! |
Changes proposed in this pull request
phase_of_matter
toSolutionArray
If applicable, fill in the issue number this pull request is fixing
Fixes #896
Checklist
scons build
&scons test
) and unit tests address code coverage