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

[Feature]: Deprecate Data.set_dataio and replace with Container.set_data_io logic #1012

Closed
CodyCBakerPhD opened this issue Dec 10, 2023 · 2 comments · Fixed by #1013
Closed

Comments

@CodyCBakerPhD
Copy link
Contributor

CodyCBakerPhD commented Dec 10, 2023

What would you like to see added to HDMF?

Data.set_dataio: https://github.com/hdmf-dev/hdmf/blob/dev/src/hdmf/container.py#L783-L790

Container.set_data_io:

hdmf/src/hdmf/container.py

Lines 749 to 753 in f95b1ef

def set_data_io(self, dataset_name, data_io_class, **kwargs):
data = self.fields.get(dataset_name)
if data is None:
raise ValueError(f"{dataset_name} is None and cannot be wrapped in a DataIO class")
self.fields[dataset_name] = data_io_class(data=data, **kwargs)

What solution would you like?

These are a bit inconsistent in their usage

I have a mild preference the previous Data method be soft deprecated in favor of the Container approach (passing both the class and the args to be passed to that class)

@CodyCBakerPhD CodyCBakerPhD changed the title [Feature]: Make Data.set_dataio more consistent with Container.set_data_io [Feature]: Deprecate Data.set_dataio and replace with Container.set_data_io logic Dec 10, 2023
@CodyCBakerPhD
Copy link
Contributor Author

Actually, after playing with it more, I don't think the Data method actually even works in practice for things like a VectorData on a H5DataIO

As seen in the tests for the set_dataio function:

def test_set_dataio(self):
"""
Test that Data.set_dataio works as intended
"""
dataio = DataIO()
data = np.arange(30).reshape(5, 2, 3)
container = Data('wrapped_data', data)
container.set_dataio(dataio)
self.assertIs(dataio.data, data)
self.assertIs(dataio, container.data)

one needs to be able to pre-initialize an H5DataIO, which is not possible as it is with the DataIO, because an H5DataIO either needs data specified (and if so cannot replace later) or dtype and shape (but if you specify these, even consistently with a future to-be-set data, you end up with an error that the data cannot be set if those other two values are not None)

@CodyCBakerPhD
Copy link
Contributor Author

Also might want to think about deprecating the HDF5IO class method, which has entirely different use (more of a convenience? But I've never seen it used...)

@classmethod
@docval(*get_docval(H5DataIO.__init__))
def set_dataio(cls, **kwargs):
"""
Wrap the given Data object with an H5DataIO.
This method is provided merely for convenience. It is the equivalent
of the following:
.. code-block:: python
from hdmf.backends.hdf5 import H5DataIO
data = ...
data = H5DataIO(data)
"""
return H5DataIO.__init__(**kwargs)

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 a pull request may close this issue.

1 participant