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.

Sign up for GitHub

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

Jump to bottom

Package & Result additions #146

Merged
merged 11 commits into from
Mar 1, 2020
Merged

Package & Result additions #146

merged 11 commits into from
Mar 1, 2020

Conversation

BvB93
Copy link
Collaborator

@BvB93 BvB93 commented Feb 28, 2020 •
edited
Loading

Major

  • Added the dill_path keyword to Package.__init__() and it's subclasses. It should take the path to a pickled plams.Results instance as value (i.e. the .dill file) and store it in the new Package.results property.
  • The Results-containing .dill file, which is contained within the Pacakge.results property, is automatically unpacked whenever getting Package.results or getting a non-existing attribute. Pacakge.results is henceforth replaced with the plams.Results instance itself and all of its non-magic/non-private methods are added to the Pacakge as instance variables.
    For example:
>>> from qmflows.packages.SCM import ADF_Result

>>> result = ADF_Result(...)
>>> print(result.results)
<scm.plams.interfaces.adfsuite.adf.ADFResults object at 0x11cc21588>

>>> print(result.get_energy)  # ``result.get_energy`` is a shortcut for ``result.results.get_energy`` 
<bound method ADFResults.get_energy of <scm.plams.interfaces.adfsuite.adf.ADFResults object at 0x11cc21588>>
  • Issue a warning when accessing Package.results yields error an encountered.
  • Added typehints to the qmflows.packages modules.

Minor

  • Updated .gitignore with ".spyproject/".
  • Exchanged all use of "/" for os.sep or os.path.join(), improving compatibility with Windows.
  • Ensure that Package and all its sub classes can handle file-like objects.
  • Improved the usage of f-strings.
  • Implemented a Package.__repr__() method.
  • Generalized the Package.__deepcopy__() so it'll also work on all its subclasses.
  • Removed empty Package.prerun() and Package.postrun() methods from Package subclasses; these (empty) methods are now already defined in the superclass.
  • find_file_pattern() now always returns an iterator; changed parse_output_warnings() to accommodate this change.
  • Cleaned up a number of docstrings
  • Made Package a proper abstract base class; Package.handle_special_keywords() and Package.run_job() are now abstract methods.

Bas van Beek added 2 commits February 28, 2020 16:34
Update packages.py
4e7dfc4 
* Make more intensive use of f-strings.
* Implemented a Package.__repr__() method.
* Added the "results" keyword to Package.__init__(); should take the pith the a pickled plams.Results instance (i.e. the .dill file).
* Generalized the Package.__deepcopy__() so it'll also work on Package subclasses.
* The Results-containing .dill file, which is contained within Pacakge.results, is automatically unpacked whenever getting Package.results or getting a non-existing attribute. Pacakge.results is henceforth replaced with the plams.Results object itself and all its non-magic/-private methods are added to the Pacakge instance as instance variables.
Update SCM.py
ce31a11 
* Implemented compatiblity with 4e7dfc4
* Changed the "/" character with os.sep (important for Windows)
@BvB93 BvB93 self-assigned this Feb 28, 2020
@BvB93 BvB93 added the Work in Progress Work in Progress label Feb 28, 2020
@BvB93 BvB93 removed the Work in Progress Work in Progress label Feb 28, 2020
@BvB93 BvB93 changed the title WiP: Package & Result additions Package & Result additions Feb 28, 2020
Bas van Beek added 5 commits February 28, 2020 18:05
Docstring improvements
62925a7
Introduced type hints and a couple other changes
f9cb0fc
Added a missing raise statement
951e990
Ensure that pkg.resource_string() always takes a string and not a fil…
917f3db 
…e-like object
Finalize; 4 changes
f40b309 
* Added type hints to all package modules.
* Cleaned up some docstrings.
* Removed empty prerun() and postrun() methods from Package subclasses; those are already defined in the superclass
* Made Package a propert abstract base class Package.handle_special_keywords() and Package.run_job() abstract methods.
@BvB93 BvB93 requested a review from felipeZ February 29, 2020 17:40
felipeZ
felipeZ approved these changes Mar 1, 2020
View reviewed changes
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.

@BvB93 Thank you for cleaning up the code! Really good work

src/qmflows/packages/SCM.py
super(ADF, self).__init__("adf")
self.generic_dict_file = 'generic2ADF.json'

def prerun(self):
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 removing this kind of things!

src/qmflows/packages/SCM.py
@@ -55,31 +60,34 @@ def run_job(settings, mol, job_name='ADFjob', nproc=None):
path_t21 = result._kf.path

# Relative path to the CWD
relative_path_t21 = '/'.join(path_t21.split('/')[-3:])
relative_path_t21 = join(*str(path_t21).split(os.sep)[-3:])
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

src/qmflows/packages/packages.py
'Package', 'run', 'registry', 'Result',
'SerMolecule', 'SerSettings']

_BASE_PATH = Path('data') / 'dictionaries'
Copy link
Contributor

Choose a reason for hiding this comment

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

great!

src/qmflows/packages/packages.py
}
del _BASE_PATH

WarnMap = Mapping[str, Type[Warning]]
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you really need a strong type language ;)

src/qmflows/packages/packages.py

def __deepcopy__(self, memo: Any) -> 'Result':
"""Return a deep copy of this instance."""
cls = type(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding this!

src/qmflows/packages/packages.py
work_dir {work_dir}\n
""")

@property
Copy link
Contributor

Choose a reason for hiding this comment

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

👊 awesome work!

src/qmflows/packages/packages.py
@@ -337,28 +434,25 @@ def run(job, runner=None, path=None, folder=None, **kwargs):
ret = call_default(job, kwargs.get('n_processes', 1),
kwargs.get('always_cache', True))
else:
raise "Don't know runner: {}".format(runner)
raise ValueError(f"Don't know runner: {runner}")
Copy link
Contributor

Choose a reason for hiding this comment

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

great catch!

@felipeZ felipeZ merged commit c8c3985 into master Mar 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felipeZ felipeZ felipeZ approved these changes

Assignees

@BvB93 BvB93

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BvB93 @felipeZ