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

Add support for verbosity keyword #74

Merged
merged 5 commits into from
May 23, 2022
Merged

Conversation

jthorton
Copy link
Contributor

Description

This PR implements and fixes #73 by adding support for a verbosity keyword which when set to muted will stop temporary files from being made which causes a failure when running many parallel xtb calculations.

Changelog description

The harness now accepts a verbosity keyword which can be a string or int corresponding to the level of verbosity, the accepted strings and associated ints are full=2, minimal=1, muted=0.

Status

Ready to go

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Thanks for sharing. I left a few comments.

xtb/qcschema/harness.py Outdated Show resolved Hide resolved
xtb/qcschema/harness.py Outdated Show resolved Hide resolved
xtb/qcschema/harness.py Outdated Show resolved Hide resolved
Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Maybe it is worth to add the string keywords directly in the interface layer at

xtb-python/xtb/interface.py

Lines 219 to 221 in d72c2ec

def set_verbosity(self, verbosity: int) -> None:
"""Set verbosity of calculation output"""
_lib.xtb_setVerbosity(self._env, verbosity)

This would keep the harness clean from this logic and allow to reuse it easily in the ASE calculator as well as any other dependent interface.

@jthorton
Copy link
Contributor Author

Thats a great idea would you want to support ints still or only the valid strings I was thinking something like this to support both and we can return the int value the verbosity is set to?

def set_verbosity(self, verbosity: Union[Literal["full", "minimal", "muted"], int]) ->int:

@awvwgk
Copy link
Member

awvwgk commented May 17, 2022

For backwards compatibility it would be preferable to still have the int support.

@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #74 (7fa18d1) into main (0a6fd5c) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
+ Coverage   97.04%   97.10%   +0.05%     
==========================================
  Files           8        8              
  Lines         440      449       +9     
==========================================
+ Hits          427      436       +9     
  Misses         13       13              
Impacted Files Coverage Δ
xtb/interface.py 97.36% <100.00%> (+0.04%) ⬆️
xtb/qcschema/harness.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a6fd5c...7fa18d1. Read the comment docs.

@jthorton
Copy link
Contributor Author

@awvwgk I think this is now ready apart from the mypy warnings, any ideas how I should get around these?

@awvwgk
Copy link
Member

awvwgk commented May 20, 2022

Seems like a false positive to me, we should be able to skip the statement with a # type: ignore comment.

@awvwgk awvwgk merged commit 0aaf169 into grimme-lab:main May 23, 2022
@jthorton jthorton mentioned this pull request Sep 8, 2022
@jthorton jthorton deleted the verbosity branch March 2, 2023 13:42
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.

Expose verbosity setting via qcshema API
2 participants