[ENH]: HDF5 please #147
No reviewers
Labels
No labels
CRITICAL
Stale
WIP
bug
concept
coordinate
dataset
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
maintenance
maps
marker
mask
on hold
parcellation
preprocess
question
ready
storage
template-space
triage
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
juaml/junifer!147
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/hdf5-storage"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Are you requiring a new dataset or marker?
Which feature do you want to include?
pretty please!
How do you imagine this integrated in junifer?
as storage
Do you have a sample code that implements this outside of junifer?
No response
Anything else to say?
No response
Codecov Report
100.00% <ø> (ø)93.91% <96.44%> (+0.29%)?Flags with carried forward coverage won't be shown. Click here to find out more.
100.00% <ø> (ø)81.81% <ø> (ø)95.92% <95.92%> (ø)100.00% <100.00%> (ø)100.00% <100.00%> (ø)100.00% <100.00%> (ø)96.36% <100.00%> (+1.82%)100.00% <100.00%> (ø)... and 1 file with indirect coverage changes
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""If there are no
kwargs, avoid having it here. I am thinking of users reading documentation.This should not be like this. If
single_outputisFalse, then it requires an element. The_collected_attribute should not exist.How is this thing collecting? How is the data in the HDF5 appended?
This still needs to be addressed.
no need for
elseclause.from the above conditions, if
feature_md5is not none, thenfeature_nameis none. Can you simplify the if conditions here?Indeed the only condition you need to check is to get the feature_md5 from the name.
The matrix in the DF format should be flattened. Indeed check what we do with matrices before storing in SQLite. This is what needs to happen now, here.
you can avoid these two if clauses with one list and then convert to tuple.
@ -0,0 +243,4 @@msg=f"`{md5}` not found in: {uri}",klass=IOError,)else:Why an
elsestatement? Are you expecting other exception that are notIOerror? What do we do with them?still needs to be addressed.
@ -0,0 +623,4 @@logger.info(f"Wrote HDF5 data for {meta_md5} to: {uri}")def store_matrix(Here's where we disagree. You are doing the same as the
SQLiteFeatureStorage. This is because we can't store a 2D matrix in SQLite, so we convert to row. The benefit of HDF5 is that we can store the 2D directly.In this function, just check for dimensions and then store the variables directly, as they are.
@ -0,0 +828,4 @@# Update metadataout_metadata.update(in_metadata)# Save metadataout_storage._write_processed_data(Why writing here? For each on of the 40k files, for each feature, you are triggering a write.
This should be done on the second step of the collect.
@ -0,0 +1,948 @@"""Provide tests for HDF5 storage interface."""See my comment on
store_matrix. We should have one row per sample. In junifer terms, one row per element.@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""I think this comment is from an earlier review, resolving it.
@ -0,0 +243,4 @@msg=f"`{md5}` not found in: {uri}",klass=IOError,)else:Looks like a comment from an earlier review, resolving it.
@ -0,0 +623,4 @@logger.info(f"Wrote HDF5 data for {meta_md5} to: {uri}")def store_matrix(From an earlier review maybe, resolving it.
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Looks like an old review, resolving it.
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Addressed in
1d220eff.@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Sorry I don't understand the reason. I am making it as explicit as possible.
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Maybe I'm not getting the rationale here, what do you mean?
@ -0,0 +828,4 @@# Update metadataout_metadata.update(in_metadata)# Save metadataout_storage._write_processed_data(Looks like an old review, resolving.
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""I know there's a bit of redundancy but kept it for explicit code. What do you mean exactly?
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""We have
**kwargsin the constructor signature for smooth subclassing and just makes it transparent to the reader that it has it. I don't have a preference here, but I have always found it in the wild and is natural to me at this point. If you have a strong reason, I don't mind.@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""If new parameters arise, then we can change the classes. We are mainly focused on user-oriented documentation, so I want to avoid having things that we don't document / are difficult to follow from the end-user point of view.
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""I meant that before this point, you already checked that only one of
feature_md5orfeature_nameare not none. There's no need for theand not feature_namehere.@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""When you convert a matrix to dataframe, it must generate one row per element. As a general rule, each DF must have only one row per element (except for timeseries).
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Do you mean to remove it from the signature as well as docstring or just the docstring?
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Addressed in
25643d91.@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Should be addressed in the latest commits.
@ -0,0 +1,948 @@"""Provide tests for HDF5 storage interface."""Addressed.
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""just the docstring will create issues with the linter no? I would go for both.
Check what we do with the SQLIteFeatureStorage. It's not that simple as creating the product, because we might only want to to store a triangular matrix.
Indeed, the code from here can be extracted to a function in storage.utils so an upper/lower/full matrix can be converted to row in the same way, in both places.
@ -0,0 +243,4 @@msg=f"`{md5}` not found in: {uri}",klass=IOError,)else:nono, it is still unresolved.
Why do you have an else statement? Do you expect other kind of error?
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Addressed in
18643864.@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""The
ndimwould be 3 here but I get your point. Addressed in07c769e4.@ -0,0 +243,4 @@msg=f"`{md5}` not found in: {uri}",klass=IOError,)else:elsein atry...except...elseis used for the condition where no exceptions are raised and the block passes.@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Addressed with latest commits.
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""doesn't work directly sending
hdf_data["element"]to the dataframe for the index?I just did this:
it works, no for loop.
same here, no need for the for loop:
@ -0,0 +363,4 @@diagonal=bool(hdf_data["diagonal"]),)# Convert data to proper 2Dreshaped_data = flat_data.TWhat happens if we
read_dfof one single element?@ -0,0 +363,4 @@diagonal=bool(hdf_data["diagonal"]),)# Convert data to proper 2Dreshaped_data = flat_data.TWorks.
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Addressed.
@ -0,0 +1,921 @@"""Provide concrete implementation for feature storage via HDF5."""Addressed.