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

Auto chunking for HDF5 #222

Merged
merged 8 commits into from
Nov 30, 2023
Merged

Auto chunking for HDF5 #222

merged 8 commits into from
Nov 30, 2023

Conversation

djps
Copy link
Collaborator

@djps djps commented Nov 3, 2023

could close #173 - it remains an open question what the default chunking should be though.

Copy link
Contributor

@peterhollender peterhollender left a comment

Choose a reason for hiding this comment

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

I like the functionality, but I think we can accomplish this all by just changing line 184 to 'chunks': True if auto_chunk else tuple(chunk_size), since chunk_size isn't used for anything other than populating opts.

My only other concern is that we haven't provided a way to invoke this option, since it isn't activated by default. Should we add the option to save_h5_file, save_file, and save_to_disk_func and kspaceFirstOrder3D? This way you could invoke it by calling output = kspaceFirstOrder3D(..., auto_chunk=True)

@waltsims
Copy link
Owner

waltsims commented Nov 7, 2023

At first glance, I see no reason to not always use auto chunking. This would greatly simplify the code.

kwave/utils/io.py Outdated Show resolved Hide resolved
kwave/utils/io.py Outdated Show resolved Hide resolved
@waltsims waltsims self-requested a review November 30, 2023 21:23
Copy link
Owner

@waltsims waltsims left a comment

Choose a reason for hiding this comment

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

looks good to me.

@waltsims
Copy link
Owner

@peterhollender @djps and I have made some changes based on your suggestions. Thanks for your input!

@waltsims waltsims merged commit aeefa26 into waltsims:master Nov 30, 2023
8 checks passed
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.

Writing hdf5 data
3 participants