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

Update dependency gemmi to >=0.5.5, <=0.5.5 #158

Merged
merged 7 commits into from
Jun 6, 2022
Merged

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Jun 2, 2022

Mend Renovate

This PR contains the following updates:

Package Change Age Adoption Passing Confidence
gemmi >=0.4.2, <=0.5.4 -> >=0.5.5, <=0.5.5 age adoption passing confidence

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

Rebasing: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 Ignore: Close this PR and you won't be reminded about this update again.


  • If you want to rebase/retry this PR, click this checkbox. ⚠ Warning: custom changes will be lost.

This PR has been generated by Mend Renovate. View repository job log here.

@renovate renovate bot added the version Version issues label Jun 2, 2022
Comment on lines 76 to 80
if return_phase_shifts:
Hasu_gemmi, ref_isym, ref_phis = gemmi_map2asu(H, sg, return_phase_shifts)
phis = np.deg2rad(rs.utils.canonicalize_phases(phic * phis))
phis = np.deg2rad(rs.utils.canonicalize_phases(phis))
else:
Hasu_gemmi, ref_isym = gemmi_map2asu(H, sg, return_phase_shifts)
Copy link
Member

Choose a reason for hiding this comment

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

Per project-gemmi/gemmi#215, Op.negated() negated the real-space translation of the symmetry operations (not just the rotational component). This had a direct impact on phase shifts, requiring them to be flipped for Friedel symmetry operations, as was done here with phic * phis.

By using op.combine(friedel), we can now directly compare the phase shifts.

@JBGreisman
Copy link
Member

Summary: this PR updates reciprocalspaceship to use gemmi v0.5.5, and sets this to the new minimal version requirement due to the following changes to gemmi namespace:

  • gemmi.GroupOps.is_centric() was changed to gemmi.GroupOps.is_centrosymmetric(). This gets called in the multiplicity calculation in rs.utils
  • gemmi.Op.negated() was removed, and we instead use gemmi.Op.combine() with the Friedel operation -x,-y,-z. As commented above, Op.negated() had also negated the real-space translation of the symmetry operations, which impacted how phase shifts were compared in one test

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.

this looks fine to me. i wanted to double check why compute_structurefactor_mutliplicity needed modifying. was go.is_centric moved to sg.is_centrosymmetric? do they do equivalent things?

@JBGreisman
Copy link
Member

JBGreisman commented Jun 6, 2022

the is_centric() method was renamed to is_centrosymmetric(). Before, only the GroupOps object had this method, but now it is in the namespace of both SpaceGroup and GroupOps objects. Here, I opted to call it directly from the SpaceGroup, but either way could have worked -- the change in method name is certainly required though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version Version issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants