Bugfix for sequence - #73
Conversation
When instantiating a sequence, usually a _result.h5-file was created to save sequence results. However, currently h5-files are only opened in read mode via h5py.File(filename, 'r') which is not creating new files if they do not exist. This is solved by first trying to open the cooresponding h5-file in read mode and is an exeption is thrown to open it in write mode. This is only added in the sequence instatiation as single shots have there shot-h5-file.
philipstarkey
left a comment
philipstarkey
left a comment
There was a problem hiding this comment.
Thanks for working on this! I've left a few comments about potential improvements to the code to make it more robust against edge-cases.
| if not group: | ||
| # Save dataset to results group by default | ||
| group = 'results/' + self.group | ||
|
|
There was a problem hiding this comment.
Could you remove this unnecessary change so that it doesn't clutter up the diff?
| self.h5_path = h5_path | ||
| self.no_write = no_write | ||
| try: | ||
| h5py.File(h5_path, 'r') |
There was a problem hiding this comment.
I think that this may leave an open reference to the h5 file. It would be better to use a context manager like this:
with h5py.File(h5_path, 'r') as f:
pass
Which ensures that the file is closed appropriately.
| self.no_write = no_write | ||
| try: | ||
| h5py.File(h5_path, 'r') | ||
| except: |
There was a problem hiding this comment.
It is best to catch a specific exception class here. Could you catch the specific exception raised? (Presumably it is FileNotFoundError?)
| try: | ||
| h5py.File(h5_path, 'r') | ||
| except: | ||
| h5py.File(h5_path,'w') |
There was a problem hiding this comment.
Similar to previous comment, use of a context manager here would be preferable. Also, you might want to consider using the 'a' filemode (so that the file doesn't get truncated should it happen to be created by an external process in between the two h5py calls executing - which could conceivably be a significant time should there be contention on the zlock/h5lock lock).
I think it might also be appropriate to wrap the creation call in a check of the no_write argument and raise the FileNotFoundError should the file not exist and the no_write argument be true?
|
Due to inactivity, I'm closing this in favour of #80 which also includes this fix. |
2 participants
When instantiating a sequence, usually a _result.h5-file was created to save sequence results. However, currently h5-files are only opened in read mode via h5py.File(filename, 'r') which is not creating new files if they do not exist. This is solved by first trying to open the cooresponding h5-file in read mode and is an exeption is thrown to open it in write mode. This is only added in the sequence instatiation as single shots have there shot-h5-file.