-
-
Notifications
You must be signed in to change notification settings - Fork 346
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 write_hdf to SolutionArray objects #680
Add write_hdf to SolutionArray objects #680
Conversation
dec7fa2
to
f39e122
Compare
Codecov Report
@@ Coverage Diff @@
## master #680 +/- ##
==========================================
+ Coverage 70.62% 70.62% +<.01%
==========================================
Files 372 372
Lines 43565 43565
==========================================
+ Hits 30768 30769 +1
+ Misses 12797 12796 -1
Continue to review full report at Codecov.
|
Does pandas require a particular HDF5 package? I don't think that's a default dependency of Pandas, but I could be wrong |
@bryanwweber ... it uses PyTables as backend. I keep a pretty sparse build tool chain, and apt-get install python3-pandas took care of everything. Haven't checked pip yet, but can look into this. |
PS: if this has legs, I am still planning on implementing unit tests, but wanted to get some feedback before getting into the CI settings. |
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.
I'm definitely in favor of this change, just some minor comments. I also don't think there's much to test here, since all of the functionality is either tested elsewhere (collect_states()
) or part of the Pandas package, which we don't really need to test.
I'm also interested in how a pip
-installed or conda
-installed Pandas manages the HDF5 dependency.
I forgot to mention in the review, there's a few places where |
f39e122
to
99f3540
Compare
Alright. pip dependencies require |
99f3540
to
5c9e348
Compare
Codecov Report
@@ Coverage Diff @@
## master #680 +/- ##
==========================================
+ Coverage 70.62% 70.62% +<.01%
==========================================
Files 372 372
Lines 43565 43565
==========================================
+ Hits 30768 30769 +1
+ Misses 12797 12796 -1
Continue to review full report at Codecov.
|
Ugh. Last push was autopep'd. Can revert if so desired. |
Please do. It makes the intentional changes hard to see in the diff. |
5c9e348
to
ffa4d3d
Compare
done. thankfully the file was mostly compliant with autopep8's choices ;) ... unintentionally opened emacs from within a conda environment with an autopep8-on-save hook enabled. |
fb802e2
to
d4f4eeb
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.
I'm in favor of this addition. My only suggested changes relate to the docstrings for the new methods.
d4f4eeb
to
99eb201
Compare
3eb9e28
to
7378cbc
Compare
@speth ... I just noticed that two issues you wanted to have addressed didn't make it into the last commit (I had taken care of it but the commit was lost). It is now fixed. |
@bryanwweber ... if this looks good to you, could you approve so I can add HDF support to #687? |
7378cbc
to
d1f1f80
Compare
* The commit implements saving of data extracted from SolutionArrays to HDF containers using pandas infrastructure. * Two methods are introduced: `write_hdf` and `to_pandas`. * Both methods only work if the pandas module can be imported; an exception is raised only if the method is called without a working pandas installation.
d1f1f80
to
9f76dec
Compare
Codecov Report
@@ Coverage Diff @@
## master #680 +/- ##
==========================================
+ Coverage 70.62% 70.63% +<.01%
==========================================
Files 372 372
Lines 43575 43575
==========================================
+ Hits 30777 30778 +1
+ Misses 12798 12797 -1
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #680 +/- ##
==========================================
+ Coverage 70.62% 70.63% +<.01%
==========================================
Files 372 372
Lines 43575 43575
==========================================
+ Hits 30777 30778 +1
+ Misses 12798 12797 -1
Continue to review full report at Codecov.
|
Please fill in the issue number this pull request is fixing:
Fixes #679
Changes proposed in this pull request:
A simple example for the intended use would be: