Skip to content
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

Introduction of PackageWrapper & ResultWrapper #149

Merged
merged 12 commits into from
Mar 4, 2020
Merged

Introduction of PackageWrapper & ResultWrapper #149

merged 12 commits into from
Mar 4, 2020

Conversation

BvB93
Copy link
Collaborator

@BvB93 BvB93 commented Mar 3, 2020

See issue #147.

Major

  • Introduced the PackageWrapper and ResultWrapper subclasses.
    PackageWrapper will take a single plams.Job type as argument and automatically return the appropiate Package instance upon calling PackageWrapper.__call__().
>>> from scm.plams import ADFJob, AMSJob, Molecule
>>> from qmflows import Settings, PackageWrapper, run

>>> from qmflows.packages.SCM import ADF_Result
>>> from qmflows.packages.package_wrapper import ResultWrapper

>>> mol = Molecule(...)
>>> s1 = Settings(...)
>>> s2 = Settings(...)

>>> job_ams = PackageWrapper(AMSJob)(s1, mol, ...)
>>> job_adf = PackageWrapper(ADFJob)(s2, mol, ...)

>>> result_ams: ResultWrapper = run(job_ams, ...)
>>> result_adf: ADF_Result = run(job_adf, ...)

  • Allow Package.generic2specific() to handle PLAMS style input settings.
    For example, settings.input.blablabla is now automatically converted into settings.specific[self.pkg_name].blablabla.

Minor

  • Changed the Package.generic_dict_file attribute from an instance- to a class-variable, as it's value is already constant with respect to the class type.
  • Changed the default value of Package.generic_dict_file to NotImplemented. Only applies to the Package baseclass, not its subclasses.
  • Got rid of a lot of nesting in Package.generic2specific().
  • Updated the Sphinx conf.py settings.
  • Changed minimum Sphinx version to 2.1.
  • Added documentation for the new PackageWrapper class.
  • Added PackageWrapper to the main __init__.py file.
  • Added new tests.
  • Bug fix in init_restart() and InitRestart: only attempt to delete the newly created workdir if there is an actual previously existing workdir.

Bas van Beek added 2 commits March 2, 2020 18:11
* Changed the ``Package.generic_dict_file`` attribute into a class variable.
* Changed the default value of ``Package.generic_dict_file`` to ``NotImplemented``.
* WiP: the ``PackageWrapper`` and ``ResultWrapper`` classes.
@BvB93 BvB93 self-assigned this Mar 3, 2020
Bas van Beek added 7 commits March 3, 2020 15:02
For example, settings.input.blablabla... is now automatically converted into settings.specific[self.pkg_name].blablablabla...
* Updated the Sphinx ``conf.py`` settings.
* Changed minimum Sphinx version to 2.1.
* Added documentation for ``PackageWrapper``.
* Added ``PackageWrapper`` to the main ``__init__.py`` file.
* Added tests.
@BvB93 BvB93 removed the Work in Progress Work in Progress label Mar 3, 2020
@BvB93 BvB93 marked this pull request as ready for review March 3, 2020 18:07
@BvB93 BvB93 requested a review from felipeZ March 3, 2020 18:07
Copy link
Contributor

@felipeZ felipeZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, Here is a conversion with @BvB93 about the reason to create these classes:
"There are more Job classes in PLAMS than the corresponding Package subclasses in QMFlows. Those could, of course, be implemented, but that can be a bit of work (which may or may not be worth it depending on the situation) Enter PackageWrapper, which is a Package subclass that can work with any arbitrary passed Job type, albeit in a more barebones fashion
Even though, when the passed Job type is recognized it will return the appropriate Package instance anyway upon calling the __call__ method For example ADFJob will default to adf, Cp2kJob return cp2k, etc.."

@@ -348,38 +359,44 @@ def generic2specific(self, settings: Settings,

specific_from_generic_settings = Settings()
for k, v in settings.items():
if k != "specific":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving this mess!

HAS_ADF: bool = ADF_ENVIRON.issubset(os.environ.keys())


def _get_result(promised_object: PromisedObject, job: Type[Job]) -> Result:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuation of #149 (comment):

Thanks for the info, I'll get back to you a bit later.

Allright, I've created a way to (sort of) mock the ADF results.
More specifically, it creates a normal-ish qmflows.Result instance and then attaches a pickled .dill file from a previous calculation.

@BvB93 BvB93 added the bug label Mar 4, 2020
@felipeZ felipeZ merged commit 414700f into master Mar 4, 2020
@BvB93 BvB93 mentioned this pull request Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants