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]: TermSetWrapper with DataIO #954

Closed
3 tasks done
mavaylon1 opened this issue Sep 6, 2023 · 4 comments
Closed
3 tasks done

[Feature]: TermSetWrapper with DataIO #954

mavaylon1 opened this issue Sep 6, 2023 · 4 comments
Assignees
Labels
category: enhancement improvements of code or code behavior priority: wontfix will not be fixed due to low priority and/or conflict with other feature/priority

Comments

@mavaylon1
Copy link
Contributor

What would you like to see added to HDMF?

The TermSetWrapper class is meant to be used with HDF5 datasets and attributes. These datasets can also be used with DataIO. If a user wants to use both, we need to provide support for that functionality. The workflow would look something like

-->H5DataIO(data=TermSetWrapper(item=data), compression=True)

This way the data is validated before going into DataIO. Once in DataIO, the wrapper would need (most likely) to be removed to not interfere with IO operations.

Is your feature request related to a problem?

No response

What solution would you like?

Refer Above.

Do you have any interest in helping implement the feature?

Yes.

Code of Conduct

@mavaylon1 mavaylon1 self-assigned this Sep 6, 2023
@mavaylon1 mavaylon1 added the category: enhancement improvements of code or code behavior label Sep 6, 2023
@oruebel
Copy link
Contributor

oruebel commented Sep 7, 2023

An alternative would be to swap the two, that is have TermSetWrapper wrap H5DataIO

TermSetWrapper(item=H5DataIO(data=data), compression=True))

In principal, I think either way is fine. With TermSetWrapper on the outside it may just work because DataIO exposes the data. With the option where TermSetWrapper is on the inside, we would probably need to unwrap the data for I/O. One option may be to update the DataIO.data property to do the unwrapping :

hdmf/src/hdmf/data_utils.py

Lines 995 to 998 in ddc842b

@property
def data(self):
"""Get the wrapped data object"""
return self.__data

@mavaylon1
Copy link
Contributor Author

An alternative would be to swap the two, that is have TermSetWrapper wrap H5DataIO

TermSetWrapper(item=H5DataIO(data=data), compression=True))

In principal, I think either way is fine. With TermSetWrapper on the outside it may just work because DataIO exposes the data. With the option where TermSetWrapper is on the inside, we would probably need to unwrap the data for I/O. One option may be to update the DataIO.data property to do the unwrapping :

hdmf/src/hdmf/data_utils.py

Lines 995 to 998 in ddc842b

@property
def data(self):
"""Get the wrapped data object"""
return self.__data

I agree either way in practice is fine, but I think implicitly it makes sense that the TermSetWrapper comes first and wraps around the data to show its validating the data and not the IO Wrapper. Though that's just a mental preference I have.

@mavaylon1
Copy link
Contributor Author

Similar to supporting iterative write with the wrapper, this is a special case that we will push back on till requested by the community.

@mavaylon1 mavaylon1 added the priority: wontfix will not be fixed due to low priority and/or conflict with other feature/priority label Oct 18, 2023
@oruebel
Copy link
Contributor

oruebel commented Oct 19, 2023

Did you test with

TermSetWrapper(item=H5DataIO(data=data), compression=True))

I think this should likely work as is and may just need a couple of unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: enhancement improvements of code or code behavior priority: wontfix will not be fixed due to low priority and/or conflict with other feature/priority
Projects
None yet
Development

No branches or pull requests

2 participants