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 interface for setting project, crystal, and dataset names #157

Merged
merged 6 commits into from
Jun 1, 2022

Conversation

JBGreisman
Copy link
Member

@JBGreisman JBGreisman commented May 31, 2022

This PR addresses #103 by adding an interface for setting the project, crystal, and dataset names in DataSet.to_gemmi() and DataSet.write_mtz(). These names are then set in the MTZ file or gemmi.Mtz object.

This can be useful for record keeping, allowing custom titles in MTZ files that can be read by other programs.

These names default to "reciprocalspaceship" -- if these new arguments are left unset, a user will have the same behavior as before.

@JBGreisman JBGreisman added the API Interface Decisions label May 31, 2022
@JBGreisman JBGreisman linked an issue May 31, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented May 31, 2022

Codecov Report

Merging #157 (97124c4) into main (79a28fb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #157   +/-   ##
=======================================
  Coverage   98.45%   98.46%           
=======================================
  Files          43       43           
  Lines        1688     1695    +7     
=======================================
+ Hits         1662     1669    +7     
  Misses         26       26           
Flag Coverage Δ
unittests 98.46% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
reciprocalspaceship/dataset.py 98.03% <100.00%> (ø)
reciprocalspaceship/io/mtz.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 79a28fb...97124c4. Read the comment docs.

Copy link
Member

@kmdalton kmdalton left a comment

Choose a reason for hiding this comment

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

I think this code would be cleaner and your tests more straightforward if the signature of to_gemmi were changed to

def to_gemmi(
        self, 
        skip_problem_mtztypes=False, 
        project_name="reciprocalspaceship",
        crystal_name="reciprocalspaceship",
        dataset_name="reciprocalspaceship",
    ):

I think strings are immutable, and this is okay to do. Let me know if I am being obtuse, and I will have another look.

@JBGreisman
Copy link
Member Author

I think that's a reasonable request. It certainly makes it more transparent what the default value will ultimately be.

@JBGreisman JBGreisman requested a review from kmdalton June 1, 2022 00:57
Copy link
Member

@kmdalton kmdalton left a comment

Choose a reason for hiding this comment

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

I think that passing DataSet.to_gemmi anything other than a string for {project,crystal,dataset}_name should raise a ValueError. Passing None should probably not be silently corrected back to the default values in the signature. I think the tests should look for a ValueError when None is passed. I might be being overly picky.

@JBGreisman
Copy link
Member Author

I'm fine with that -- I agree that silently coercing a value of None to "reciprocalspaceship" is a bit surprising from an interface perspective. The docstrings certainly specify that the values take str, so I think it's reasonable to raise a ValueError for anything else.

@JBGreisman JBGreisman requested a review from kmdalton June 1, 2022 16:07
Copy link
Member

@kmdalton kmdalton left a comment

Choose a reason for hiding this comment

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

I think that the methods DataSet.to_gemmi and DataSet.write_mtz should be the only places with default arguments for the various name strings. The functions in io are not really meant to be called directly by users. I think it is good for them to have the names as required arguments. This way, if we ever decide to change the default names in the future, they only need to be updated at the top level.

@JBGreisman JBGreisman requested a review from kmdalton June 1, 2022 17:23
Copy link
Member

@kmdalton kmdalton left a comment

Choose a reason for hiding this comment

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

i don't see any glaring issues here, and i like the solution.

@JBGreisman JBGreisman merged commit 2e24be9 into main Jun 1, 2022
@JBGreisman JBGreisman deleted the mtz_names branch June 1, 2022 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Interface Decisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MTZ data set hierarchy?
3 participants