Skip to content
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 append for Datasets in Data objects #161

Merged
merged 5 commits into from
Oct 2, 2019
Merged

Conversation

bendichter
Copy link
Contributor

change table to use the newly defined append method

needed for NeurodataWithoutBorders/pynwb#1067

* change table to use the newly defined append method
src/hdmf/container.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

Merging #161 into dev will increase coverage by 0.05%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #161      +/-   ##
==========================================
+ Coverage   69.85%   69.91%   +0.05%     
==========================================
  Files          30       30              
  Lines        5756     5767      +11     
  Branches     1353     1355       +2     
==========================================
+ Hits         4021     4032      +11     
+ Misses       1307     1302       -5     
- Partials      428      433       +5
Impacted Files Coverage Δ
src/hdmf/common/table.py 53.61% <66.66%> (ø) ⬆️
src/hdmf/container.py 68.24% <81.81%> (+1.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7029e3a...514fbcf. Read the comment docs.

Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have tests for these new cases for append. Other than that this looks good to me.

@rly
Copy link
Contributor

rly commented Oct 2, 2019

Suggestion: on line 58 of src/hdmf/common/table.py: change self.data.append to self.append
Otherwise, we will have a similar problem that we had before where self.data is an h5py.Dataset and h5py.Dataset does not have the function append.

@bendichter
Copy link
Contributor Author

@oruebel yeah, we should write some tests in hdmf. I just wanted to get this in the queue so the whole team could be on board with these changes.

@bendichter
Copy link
Contributor Author

I added the tests to test_io_map_data.py as this was the only place in the tests I found an instantiated Data.

@bendichter
Copy link
Contributor Author

@oruebel is this OK to merge?

Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bendichter looks good. Just FYI, this change introduces an issue in that with this Data is now dependent on the HDF5 backend. This means., if we add new backends the changes are no longer contained to a new HDMFIO class, but we'd need to update Data as well. However, this is a non-trivial issue. For now, lets merge this and we'll worry about fixing this as we work on getting the Zarr backend ready for merge.

@bendichter
Copy link
Contributor Author

@oruebel Yes, I agree it would be better to define these methods in a way that is supportable within the different backend modules once we are supporting other backends.

@bendichter bendichter merged commit 9a5e2e7 into dev Oct 2, 2019
@rly rly deleted the enh/append_dataset branch October 16, 2019 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants