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

add flush option to hdf5 write_metadata #1455

Merged
merged 7 commits into from
Feb 11, 2019

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Jan 28, 2019

Fixes issue with HDF5 files that are not properly closed.

When metadata is written after the main data has been written, the HDF5 file was properly closed. This could lead to currupt HDF5 files. This PR fixes the issue by flushing the HDF5 file by default after writing the metadata.

@jenshnielsen @WilliamHPNielsen

@astafan8
Copy link
Contributor

Could you expand on what this PR is about and in what situation the problem happens? Why wouldn't a file be "not properly closed"? And what issue exactly occurs when "a file is not properly closed"?

Also, this change broke some tests, so, please, fix them. Ideally, the change in the test will help communicating why the new behavior is more beneficial.

@eendebakpt
Copy link
Contributor Author

@astafan8 I updated the PR description. The failing test was invalid, so I removed it.

The test checked whether copying a generated HDF5 file without performing an explicit flush would generate an error. Although copying the file without an explicit flush can result in an error, this is not necessary (since the flush can be triggered by other methods as well).

@codecov
Copy link

codecov bot commented Jan 29, 2019

Codecov Report

Merging #1455 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1455      +/-   ##
==========================================
+ Coverage   73.86%   73.86%   +<.01%     
==========================================
  Files          92       92              
  Lines       10410    10411       +1     
==========================================
+ Hits         7689     7690       +1     
  Misses       2721     2721

@peendebak
Copy link
Contributor

@astafan8 The PR passes all tests.

@astafan8 astafan8 merged commit bec01dc into microsoft:master Feb 11, 2019
giulioungaretti pushed a commit that referenced this pull request Feb 11, 2019
Merge: 67d0c83 ff16f88
Author: Mikhail Astafev <astafan8@gmail.com>

    Merge pull request #1455 from VandersypenQutech/fix/hdf5
@peendebak peendebak deleted the fix/hdf5 branch March 22, 2019 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants