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.

Sign up for GitHub

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

Jump to bottom

Add public API to read and write HDF5 #4

Merged
merged 8 commits into from
Sep 19, 2019
Merged

Add public API to read and write HDF5 #4

merged 8 commits into from
Sep 19, 2019

Conversation

rejsmont
Copy link
Collaborator

  • DataSetInfo os now in its own namespace.
  • HDF5ImageJ now has hdf5list method to list contents of HDF5 file
  • hdf5read and hdf5write methods are now more flexible (writers take ImagePlus as argument)
  • HDF5_Reader_Vibez now uses abstracted DataSetInfo class
  • Proposed version bump to 0.2.0 (as new features were added)

@rejsmont
Add public API to read and write HDF5.
62a1c6a 
* DataSetInfo os now in its own namespace.
* HDF5ImageJ now has `hdf5list` method to list contents of HDF5 file
* `hdf5read` and `hdf5write` methods are now more flexible
* HDF5_Reader_Vibez now uses abstracted DataSetInfo class
@rejsmont
Version bump (extended API)
345a8c1
@rejsmont
Writers take ImagePlus as argument
8e87aa5
@rejsmont
Me stupido!
4254c74
@rejsmont
Do not show imp when using new API functions...
c5a1d25 
...old functions behave as previously.
@rejsmont rejsmont mentioned this pull request Mar 22, 2018
Open
@rejsmont
Copy link
Collaborator Author

This should close #1 :)

@rejsmont
Copy link
Collaborator Author

Please wait with merging - apparently this somehow fails when HDF5 has more than one dataset. Working this out now...

@rejsmont
Copy link
Collaborator Author

OK - it is now fixed and tested with several HDF5 files!

@ctrueden
Copy link
Member

ctrueden commented Jul 2, 2018

@rejsmont Thank you very much for the patches, and sorry for the long delay in reply!

@kmdouglass What do you think? Do you have time to review this?

@rejsmont
Copy link
Collaborator Author

Need to include changes introduced by @GFleishman before merging.

@rejsmont
Copy link
Collaborator Author

rejsmont commented Oct 5, 2018

@GFleishman could you please review this PR? Please make sure that links functionality is preserved. Thanks in advance!

@ctrueden
Copy link
Member

Thanks again @rejsmont, the patch looks good to me. Since it essentially all new API, the chances of breaking backwards compatibility are less. (As far as I could see, no existing public API was broken or changed... right?)

The main thing missing is unit tests, but it's really up to you whether you have time to write any. If you do not have any tests, then future breakages will be harder to detect.

@rejsmont
Copy link
Collaborator Author

Accepting this even though no unit tests are present. Will add issue to add unit tests ASAP.

@rejsmont rejsmont closed this Sep 19, 2019
@rejsmont rejsmont reopened this Sep 19, 2019
@rejsmont rejsmont merged commit b581c70 into fiji:master Sep 19, 2019
ctrueden added a commit that referenced this pull request Oct 23, 2019
@ctrueden
POM: update minor version digit
b3bd10d 
New API was introduced by #4. While that PR did bump the version
(345a8c1), the branch was old, so the
change did not persist as a SemVer bump for the current version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rejsmont @ctrueden