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 decorators for gemmi argument checking #94

Merged
merged 9 commits into from
Sep 14, 2021
Merged

Add decorators for gemmi argument checking #94

merged 9 commits into from
Sep 14, 2021

Conversation

JBGreisman
Copy link
Member

Fixes #92.

It is common in rs to write functions that take gemmi.SpaceGroup or gemmi.UnitCell instances as arguments. In order to make these methods more flexible, it is often useful to add error checking code so that if an int is provided instead of a gemmi.SpaceGroup, the method generates the equivalent gemmi object instance. This was done for the DataSet.cell and DataSet.spacegroup setter methods to address #18.

This PR generalizes this idea using @spacegroupify and @cellify decorators to avoid needing to write boiler plate argument checking code. This allows one to write a function as if it is guaranteed that the input argument is a gemmi object, while still being able to input that argument flexibly. For example:

from reciprocalspaceship.decorators import spacegroupify

@spacegroupify
def get_xhm(spacegroup=None):
    return spacegroup.xhm()

import gemmi
print(get_xhm(gemmi.SpaceGroup("1"))) # prints 'P 1'
print(get_xhm(1))                     # prints 'P 1'
print(get_xhm("P1"))                  # prints 'P 1'

@JBGreisman JBGreisman added API Interface Decisions enhancement Improvement to existing feature labels Sep 13, 2021
@JBGreisman
Copy link
Member Author

JBGreisman commented Sep 13, 2021

To do:

  • Use decorators in relevant parts of the rs codebase

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2021

Codecov Report

Merging #94 (9fb3a52) into main (dfdae4e) will increase coverage by 0.09%.
The diff coverage is 99.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
+ Coverage   98.87%   98.97%   +0.09%     
==========================================
  Files          40       41       +1     
  Lines        1512     1561      +49     
==========================================
+ Hits         1495     1545      +50     
+ Misses         17       16       -1     
Flag Coverage Δ
unittests 98.97% <99.03%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
reciprocalspaceship/decorators.py 98.61% <98.61%> (ø)
reciprocalspaceship/dataset.py 99.01% <100.00%> (+0.17%) ⬆️
reciprocalspaceship/utils/asu.py 100.00% <100.00%> (ø)
reciprocalspaceship/utils/cell.py 100.00% <100.00%> (ø)
reciprocalspaceship/utils/phases.py 100.00% <100.00%> (ø)
reciprocalspaceship/utils/stats.py 100.00% <100.00%> (ø)
reciprocalspaceship/utils/structurefactors.py 100.00% <100.00%> (+2.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 dfdae4e...9fb3a52. Read the comment docs.

Comment on lines -78 to -87
if isinstance(sg, SpaceGroup):
group_ops = sg.operations()
elif isinstance(sg, GroupOps):
group_ops = sg
else:
raise ValueError(
f"gemmi.SpaceGroup or gemmi.GroupOps expected for parameter sg. "
f"Received object of type: ({type(sg)}) instead."
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: This change removes support for invoking this function directly with a GroupOps object. We do not actually use this anywhere in our codebase, so it seems like the added flexibility of @spacegroupify is preferable. This is also true for is_absent() and is_centric().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Interface Decisions enhancement Improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] gemmification decorators
2 participants