search

aldanor / hdf5-rust

HDF5 for Rust
https://docs.rs/hdf5
Apache License 2.0
310 stars 85 forks source link

The DCPL branch #140

Closed mulimoen closed 3 years ago

mulimoen commented 3 years ago

This is a continued effort on the dcpl-branch of @aldanor. This adds a lot more functionality and makes the API much nicer to use for selections.

See mulimoen#2 for some earlier comments and discussions regarding this branch.

Fixes #45 Fixes #88 Fixes #126

TheButlah commented 3 years ago

This PR looks super useful, but is it abandoned now?

aldanor commented 3 years ago

This PR looks super useful, but is it abandoned now?

In brief: nope.

aldanor commented 3 years ago

Great, thanks. I think I've finished skimming over the code in general - let me re-read the actual dataset / dataset-builder part today and I'll send some comments on that if any. I think after that we're good to go.

(I wouldn't make a release before probably merging in #139 too since it's quite a big change - will get back re: that as well)

aldanor commented 3 years ago

@mulimoen I've went through the code once again and (the lack of docs and docstrings aside, and also the complete lack of tests aside) it's pretty good to go. I left a few minor comments here and there.

(I really think we should write some docs and tests after this is released and is considered stable api-wise.)

mulimoen commented 3 years ago

I've opened some new GH issues for some of the remaining tasks. This PR is enourmous as it is, and the other issues can be merged and reviewed separately.

aldanor commented 3 years ago

Good point re: separate tickets (array trait, docs, avoiding panics), they can be handled separately. Maybe also a ticket for adding full tests for dataset and all related builders etc? A lot of the code in this PR is fully covered by tests (independent parts like extents, selection, dyn value, etc). What needs to be thoroughly tested is all of the new builders and the new Dataset - also lzf/blosc compression and all the new features.

aldanor commented 3 years ago

At this point I'm somewhat scared to press the green button because this PR is so big my browser sometimes hangs when viewing diffs :) Great work @mulimoen in resurrecting it and finishing all the unfinished parts!

(Before the release, as I've said, I think we should consider merging at least the ownership/IDs PR)

aldanor commented 3 years ago

(Added a note that this fixes #47 as well)

mulimoen commented 3 years ago

(Added a note that this fixes #47 as well)

Only halfway, one would need to dispatch based on some fixed sizes.

aldanor commented 3 years ago

(Added a note that this fixes #47 as well)

Only halfway, one would need to dispatch based on some fixed sizes.

Hm? The question was to create a dataset with runtime-known string size. This is now doable via empty_as() and with_data_as()?

mulimoen commented 3 years ago

I think what @pmarks is trying is creating the type from an n that is only known at runtime (max of all strings), but one still needs to define this type at compile time to use at runtime. The workaround in https://github.com/aldanor/hdf5-rust/issues/47#issuecomment-641915316 is using a giant match to work around this by letting each arm have the compile time fixed value. I'll remove the issue so it's not closed on merge.

mulimoen commented 3 years ago

Thank you very much for reviewing and finishing this @aldanor! Hopefully future PRs won't be as massive as this one!

aldanor commented 3 years ago

Yea - well we solved it halfway by allowing runtime descriptors, that's already something that wasn't previously possible, so that's something.

Also, that 'giant match' can now be implemented via const generics.

Finally, with strings... I have a ticket with a hacky solution for Rust types like strings - maybe we'll get to that at some point as well.

aldanor commented 3 years ago

Thank you very much for reviewing and finishing this @aldanor! Hopefully future PRs won't be as massive as this one!

Yea, let's try to keep it simple from now on :) I may open a few PRs in the nearby future with some docs updates.