Skip to content

Conversation

@rburghol
Copy link
Contributor

@rburghol rburghol commented Jan 30, 2026

Closed in favor of cleaner git log in #217

@rburghol rburghol changed the title Saved2 Pandas 3.0, create new HDF5 close() method Jan 30, 2026
Copy link
Contributor

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.

Copy link
Contributor

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()
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@rburghol rburghol mentioned this pull request Jan 31, 2026
8 tasks
@rburghol rburghol closed this Jan 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants