HDF5 Driver Update #550
HDF5 Driver Update #550
Conversation
Pull Request Test Coverage Report for Build 1881110027
💛 - Coveralls |
mrossinek
requested review from
stefan-woerner,
pbark,
manoelmarques and
woodsp-ibm
as code owners
mrossinek
force-pushed
the
hdf5_driver_update
branch
from
adba73d to
6c7dcfd
Compare
|
I rebased this PR on #554, blocking it until that is merged. |
mrossinek
force-pushed
the
hdf5_driver_update
branch
from
4177637 to
4a78de9
Compare
mrossinek
force-pushed
the
hdf5_driver_update
branch
from
4a78de9 to
da1bec7
Compare
| @@ -62,8 +74,64 @@ def run(self) -> ElectronicStructureDriverResult: | |||
| if not os.path.isfile(hdf5_file): | |||
| raise LookupError(f"HDF5 file not found: {hdf5_file}") | |||
|
|
|||
| return hdf5_file | |||
|
|
|||
| def convert(self, replace: bool = True) -> None: | |||
There was a problem hiding this comment.
I am wondering about this being an instance method, but more so the default behavior of it overwriting of the same file is destructive if anything goes wrong or not what the user might have wanted. Something safer by default I think.
Now would this be better as a static method or function in this same module that takes the input file name, an optional output file name and replace False by default. So it becomes more of a conversion function that can be used that is not done as really an extension of driver functionality. By default the output file name could be the input file name part with _new appended. E.g I give it "h2_631g.hdf5" which is a legacy hdf5 and it would create, by default, "h2_631g_new.hdf5", unless I give it an explicit name. If the output already exists, for whatever, it would require them to set replace to True. Buyer beware I guess if you give the output file the same name as the input and replace True. But at least it would not be the default behavior where they may lose their input file with it being overwritten and intended to keep it. If the input is not a legacy hdf5 file the raise an error indicating that etc.
If you prefer to keep this as an instance method, supplying the source file to be converted via the constructor I the behavior could still be the same as described for the function above - just that the source name would not need to be provided.
There was a problem hiding this comment.
I have updated the default value of replace and the HDF5 suffix handling.
However, I kept convert() as an instance method. My reasoning is that a user may encounter the warning of using a legacy file at runtime where they already instantiated the driver object. They can then simply run driver.convert() to update the legacy file.
mrossinek
force-pushed
the
hdf5_driver_update
branch
from
0753cd5 to
5766690
Compare
* refactor: basic HDF5Driver update * test: test both HDF5 type supports * Fix copyright * test: update new expected HDF5 file * ref: align new HDF5Driver with qiskit-community#554 * feat: add HDF5Driver.convert utility method * Add reno * Fix linters * refactor: do not replace by default * refactor: use `_new.hdf5` instead of `.hdf5.new` * refactor: update wording * fix: Python <3.9 compatibility * Update docstring Co-authored-by: Panagiotis Barkoutsos <[email protected]>
Summary
Refactors the
HDF5Driverto be aligned with #461.It will also support loading legacy QMolecule files stored in HDF5 files.
Details and comments