-
Notifications
You must be signed in to change notification settings - Fork 209
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
Allow custom Property
integration
#312
Comments
This issue will become resolvable once #406 is merged. However, even then, using the feature proposed in this issue will always require dictionary-based Anyways, details aside, once we can ensure dict-based Such a use case will always be limited in potential with regards to arbitrary transformer support, since a transformer may not know how to handle a certain property. This caveat should be possible to workaround in most scenarios though, especially via leveraging the |
The README test previously failed because the iteration over the auxiliary operator observables in the ElectronicStructureResult is currently unable to handle the lack of certain properties which have always been evaluated for legacy reasons (AngularMomentum, Magnetization). Even if we were to default them to an empty list instead of None, while the zip command would execute normally, no results would be printed since zip stops after the shortest length. That being said, fixing ElectronicStructureResult is not the solution right now in any case, since a user would be unable to manually request the computation of AngularMomentum and Magnetization before we resolve the issue ý¿¿¼£�qiskit-community#312 Thus, this commit reverts the exclusion of these auxiliary operators in the case of `settings.dict_aux_operators`.
* WIP: HDF5-integration for property framework This is an initial proof-of-concept implementation. This needs to be reviewed extensively. * Clean up interface * refactor: let ElectronicIntegrals derive from PseudoProperty * refactor: let Molecule derive from PseudoProperty * feat: implement DriverMetadata HDF5 methods * feat: add toggle to include PseudoProperty objects in GroupedProperty iteration * feat: implement ParticleNumber HDF5 methods * feat: implement AngularMomentum and Magnetization HDF5 methods * feat: implement ElectronicBasisTransform HDF5 methods * fix: lint * fix: fix unittests * remove comment * fix: spell * feat: also store Qiskit Nature version * handle potential error cases in Property.import_and_build_from_hdf5 * Fix copyright * feat: introduce individual version numbers per Property class * WIP: HDF5 integration into vibrational properties * Fix copyright * fix: property tutorial * Update typehints * Run black * Remove @AbstractMethod from Property.from_hdf5 This needs to be removed in order to ensure that the stable tutorials remain working. * Add more missing typehints * refactor: introduce HDF5Storable Protocol * refactor: remove PseudoProperty in favor of Interpretable Protocol The PseudoProperty class effectively removes everything which defines the Property class (the interpret method). So instead of having such a pseudo-class, all previous PseudoProperty subclass are now directly Property subclasses and the `interpret()` method existence is handled via the `Interpretable` Protocol. * Fix linters * Fix ASTransformer caught error type * Fix copyright * Fix imports * More guards against Property type * More import fixes * fix: property tutorial * fix: ElectronicStructureDriverResult.__str__ * refactor: remove Property base class where not needed * test: basic hdf5 method unittests * test: *StructureDriverResult from_hdf5 methods * docs: HDF5 documentation * refactor: formally deprecate PseudoProperty class * docs: actual HDF5 save and load examples * Fix spell * fix: avoid name clash with multiple atoms of same kind * fix: ElectronicEnergy.from_hdf5 group access * Update unittest HDF5 resource * fix: ParticleNumber.from_hdf5 occupation dataset access * Fix #519 This is actually required for the matrices loaded during ElectronicIntegrals.from_hdf5 to be in the correct order! * Update qiskit_nature/properties/property.py Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> * Update docs * Rename save_to_hdf5(..., force -> replace) * refactor: fix DriverMetadata HDF5 attribute names * refactor: make from_hdf5 a staticmethod * feat: store Molecule.units in HDF5 * fix: update expected HDF5 result * docs: include backwards compatibility expectations * feat: add skip_unreadable_data toggle to HDF5 loading methods * Fix spell Apparently Sphinx can now use `kwds` instead of `kwargs` * docs: explicitly request error raising * docs: use `:func:` instead of `:class:` * docs: ensure *StructureDriverResults are documented * refactor: enforce keyword arguments in hdf5 module * Update driver return types While the previous return types were not wrong, for documentation purposes using these concrete implementations is a bit nicer. * Run black * Add reno * feat: use Molecule.units during from_hdf5 * test: refactor _hdf5 method tests Instead of hard-coding the expected HDF5 file structure, we test that `to_hdf5` and `from_hdf5` work consistently with each other. The `test_to_hdf5` tests ensure that this method executes correctly (i.e. without errors). The `from_hdf5` tests ensure that first writing and subsequently reading a property from a file produces an identical instance. In the future, once version numbers of certain properties may increase, we should store HDF5 files and compare those against expected instances. * fix: README test The README test previously failed because the iteration over the auxiliary operator observables in the ElectronicStructureResult is currently unable to handle the lack of certain properties which have always been evaluated for legacy reasons (AngularMomentum, Magnetization). Even if we were to default them to an empty list instead of None, while the zip command would execute normally, no results would be printed since zip stops after the shortest length. That being said, fixing ElectronicStructureResult is not the solution right now in any case, since a user would be unable to manually request the computation of AngularMomentum and Magnetization before we resolve the issue ý¿¿¼£�#312 Thus, this commit reverts the exclusion of these auxiliary operators in the case of `settings.dict_aux_operators`. * refactor: use only public API in PropertyTest * refactor: update type hints * fix: update TestVibrationalStructureDriverResult to G16 Rev.C01 Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
I gave this some more thought and wanted to query @pbark and @woodsp-ibm for the following ideas:
|
Hopefully I understood the intent of the above; anyway here is my thought back
Are we always able to do this? If they are dependent on the problem being further processed, which I think what we generate today are e.g. at least going through the driver to get more information on the problem, does it work at that level in general? The current properties are the result of the driver being run, they are not formulated on the input, rather some entity, which is part of the problem configuration supplied to the init, i.e. the driver, produces them - they are not directly supplied as such.
A generator is pretty much saying one supports a list right - albeit that its a bit more lazily done. In the past in Aqua VQE supported aux_ops as a single instance or a list so as to be convenient in that the user did not have to wrap it in a list if they just had a single instance (not that its of course must to actually do that). Maybe a Generator is what you had in mind for the above so it can be input and only lazily created on demand later - but isn;t information that it might need still an issue.
How do you define already present? I think internally we store these via their name in some map. Hence a unique name would allow it to be stored whereas presumably storing something with the same name (key) would overwrite any existing one. If someone came up with different implementation of something we have is that supported/ok if its called by the same name. I believe we expect/refer to them by name to get them from the current driver result group. In a regular Python dict I any object that existed is overwritten by a new one with the same key right. That seems to be what happens today when you add a property. (Just as an aside I guess the reason the GroupedProperty does not implement Mapping is because we get the key from the item, it name or class name). As a thought the other way, although this is more about customizing the built-in property behavior rather than strictly custom properties, one of the things we had talked about in the past when having more granular properties was allowing a user to say exclude dipole, or some other other properties from the computation. Though of course in practice the overhead is just some creation of smaller operators and a single evaluation at the result state(s). |
No, this is not generally possible. As you point out correctly, cases like e.g. the
This is not quite what I meant. In general I think we will support def construct_magnetization(driver_result: ElectronicStructureDriverResult) -> Magnetization:
num_spin_orbitals = driver_result.get_property("ParticleNumber").num_spin_orbitals
return Magnetization(num_spin_orbitals)
problem = ElectronicStructureProblem(driver, transformers=[], aux_properties=[construct_magnetization]) This is a very minimal example but more fancy operations could be done, too.
Yes this is correct. And you're right I guess a user can use an identical name if they want to overwrite, and a different one if they do not want to do that. This will need clear documentation.
This is a very good point actually. Maybe we do not need to provide def extend_properties(driver_result: ElectronicStructureDriverResult) -> None:
num_spin_orbitals = driver_result.get_property("ParticleNumber").num_spin_orbitals
driver_result.add_property(Magnetization(num_spin_orbitals))
driver_result.pop("ElectronicDipoleMoment")
problem = ElectronicStructureProblem(driver, transformers=[], property_cb=extend_properties) |
In the design described in #701 this feature will directly be available without further changes. Thus, I am closing this issue as it is superseded by that new design. |
* WIP: HDF5-integration for property framework This is an initial proof-of-concept implementation. This needs to be reviewed extensively. * Clean up interface * refactor: let ElectronicIntegrals derive from PseudoProperty * refactor: let Molecule derive from PseudoProperty * feat: implement DriverMetadata HDF5 methods * feat: add toggle to include PseudoProperty objects in GroupedProperty iteration * feat: implement ParticleNumber HDF5 methods * feat: implement AngularMomentum and Magnetization HDF5 methods * feat: implement ElectronicBasisTransform HDF5 methods * fix: lint * fix: fix unittests * remove comment * fix: spell * feat: also store Qiskit Nature version * handle potential error cases in Property.import_and_build_from_hdf5 * Fix copyright * feat: introduce individual version numbers per Property class * WIP: HDF5 integration into vibrational properties * Fix copyright * fix: property tutorial * Update typehints * Run black * Remove @AbstractMethod from Property.from_hdf5 This needs to be removed in order to ensure that the stable tutorials remain working. * Add more missing typehints * refactor: introduce HDF5Storable Protocol * refactor: remove PseudoProperty in favor of Interpretable Protocol The PseudoProperty class effectively removes everything which defines the Property class (the interpret method). So instead of having such a pseudo-class, all previous PseudoProperty subclass are now directly Property subclasses and the `interpret()` method existence is handled via the `Interpretable` Protocol. * Fix linters * Fix ASTransformer caught error type * Fix copyright * Fix imports * More guards against Property type * More import fixes * fix: property tutorial * fix: ElectronicStructureDriverResult.__str__ * refactor: remove Property base class where not needed * test: basic hdf5 method unittests * test: *StructureDriverResult from_hdf5 methods * docs: HDF5 documentation * refactor: formally deprecate PseudoProperty class * docs: actual HDF5 save and load examples * Fix spell * fix: avoid name clash with multiple atoms of same kind * fix: ElectronicEnergy.from_hdf5 group access * Update unittest HDF5 resource * fix: ParticleNumber.from_hdf5 occupation dataset access * Fix qiskit-community#519 This is actually required for the matrices loaded during ElectronicIntegrals.from_hdf5 to be in the correct order! * Update qiskit_nature/properties/property.py Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> * Update docs * Rename save_to_hdf5(..., force -> replace) * refactor: fix DriverMetadata HDF5 attribute names * refactor: make from_hdf5 a staticmethod * feat: store Molecule.units in HDF5 * fix: update expected HDF5 result * docs: include backwards compatibility expectations * feat: add skip_unreadable_data toggle to HDF5 loading methods * Fix spell Apparently Sphinx can now use `kwds` instead of `kwargs` * docs: explicitly request error raising * docs: use `:func:` instead of `:class:` * docs: ensure *StructureDriverResults are documented * refactor: enforce keyword arguments in hdf5 module * Update driver return types While the previous return types were not wrong, for documentation purposes using these concrete implementations is a bit nicer. * Run black * Add reno * feat: use Molecule.units during from_hdf5 * test: refactor _hdf5 method tests Instead of hard-coding the expected HDF5 file structure, we test that `to_hdf5` and `from_hdf5` work consistently with each other. The `test_to_hdf5` tests ensure that this method executes correctly (i.e. without errors). The `from_hdf5` tests ensure that first writing and subsequently reading a property from a file produces an identical instance. In the future, once version numbers of certain properties may increase, we should store HDF5 files and compare those against expected instances. * fix: README test The README test previously failed because the iteration over the auxiliary operator observables in the ElectronicStructureResult is currently unable to handle the lack of certain properties which have always been evaluated for legacy reasons (AngularMomentum, Magnetization). Even if we were to default them to an empty list instead of None, while the zip command would execute normally, no results would be printed since zip stops after the shortest length. That being said, fixing ElectronicStructureResult is not the solution right now in any case, since a user would be unable to manually request the computation of AngularMomentum and Magnetization before we resolve the issue ý¿¿¼£�qiskit-community#312 Thus, this commit reverts the exclusion of these auxiliary operators in the case of `settings.dict_aux_operators`. * refactor: use only public API in PropertyTest * refactor: update type hints * fix: update TestVibrationalStructureDriverResult to G16 Rev.C01 Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
What is the expected enhancement?
After #303 will be merged, it will not yet be possible to add custom
Property
objects to theGroupedProperty
object used within theBaseProblem
implementations. A major reason for holding off on this, is that we really need Qiskit/qiskit#6772 to be implemented. Otherwise, a lot of edge cases can break down when we allow arbitrary sets of properties.As of Qiskit Nature 0.2.0, the tutorial on the Property framework will explain how to implement a custom Property and contain a note linking to this issue. A workaround by means of passing the operators of interest as
aux_operators
to the solver will be included.The text was updated successfully, but these errors were encountered: