-
Notifications
You must be signed in to change notification settings - Fork 20
Pandas 3.0, create new HDF5 close() method #216
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!! I'm not sure how this fits in with the pandas 3.0 patch, but we should have a look at this as a team, for sure. Both implementations break an important (and oft un-enforced in non-aerospace grade code) rule to never branch on bare float equality tests, instead it's safer to set a tolerance and check against it via abs(a - b) > tol or whatever. This would be a huge undertaking for us to track down comprehensively for our code base, but maybe we do it whenever we find cases like this where our code branches differently from the fortran and then our tests fail. This is another great reason to promote the checks in the cmd_regression into the test suite, if they're not there already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This edit is not needed since the issue is on the caller side not using this class as a context manager. That said, an explicit close method is a fine thing to add, but we should be guiding folks to use the with statement rather than manually closing things.
| hdf5_instance = HDF5(h5file) | ||
| io_manager = IOManager(hdf5_instance) | ||
| main(io_manager, saveall=saveall, jupyterlab=compress) | ||
| hdf5_instance.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should promote this to a with statement:
with HDF5(h5file) as hdf5_instance:
io_manager = IOManager(hdf5_instance)
main(io_manager, saveall=saveall, jupyterlab=compress)this handles opening and closing all in one. even better would be to wrap this all in the IO manager, and have that be the context manager.
I'm pretty sure this edit would work without changing any of the hsp2io.hdf code too, it seems to already be written to be used as a context manager like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heard and implemented. This functions fine and passes tests, and also, as you note avoids a stuck file if something catastrophic happens during runtime.
Closed in favor of cleaner git log in #217