Conversation
Reviewer's GuideThe Sequence diagram for sym_redock with mask parametersequenceDiagram
participant C as Caller
participant SR as sym_redock
participant CEC as clash_error_comp
participant LBFGS as lbfgs
C->>+SR: sym_redock(xyz, Lasu, frames, opt, mask)
note right of SR: 'mask' is now an optional parameter
SR->>SR: Initialize optimization parameters (Q0, T0)
SR->>LBFGS: Run optimization using 'closure'
loop Optimization Steps
LBFGS->>+SR: closure()
SR->>+CEC: clash_error_comp(..., mask)
note right of CEC: If mask is provided...
CEC->>CEC: Modify clash calculation (dsymm_2) to ignore masked residues
CEC-->>-SR: loss
SR-->>-LBFGS: loss
LBFGS->>LBFGS: Update parameters (Q0, T0)
end
SR-->>-C: Optimized coordinates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Summary
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new flag to control the behavior of sym_redock so that it is applied only on diffused residues while leaving motifs at the same xyz.
- Added an optional mask parameter to the sym_redock function.
- Modified the clash_error_comp function to accept and apply the mask during clash calculations.
ipd/sym/sym_util.py
Outdated
| def clash_error_comp(R0, T0, xyz, fit_tscale): | ||
| def clash_error_comp(R0, T0, xyz, fit_tscale, mask=None): | ||
| if mask is not None: | ||
| mask0 = mask[:Lasu.item()] |
There was a problem hiding this comment.
If Lasu is already an integer type, calling .item() will raise an error. Consider using Lasu directly or ensuring that Lasu is a tensor before calling .item().
| mask0 = mask[:Lasu.item()] | |
| mask0 = mask[:Lasu.item()] if isinstance(Lasu, th.Tensor) else mask[:Lasu] |
ipd/sym/sym_util.py
Outdated
| mask = ~(mask0[None, :].expand(Lasu, Lasu)) | ||
| mask = mask | mask.T | ||
| # dsymm_2 = dsymm.clone().fill_diagonal_(9999) # avoid in-place operation | ||
| for i in range(0, len(Xsymmall), Lasu): | ||
| dsymm_2[i:i + Lasu, i:i + Lasu] = 9999 | ||
| for i in range(0, len(Xsymmall), Lasu): # loop over rows | ||
| dsymm_2[i:i + Lasu, i:i + Lasu] = 9999 # masking intra-contact | ||
| if mask is not None: | ||
| for j in range(0, len(Xsymmall), Lasu): # loop over cols | ||
| if j == i: continue | ||
| # masking inter-contacts involving masked residues | ||
| dsymm_2[i:i + Lasu, j:j + Lasu][mask] = 9999 |
There was a problem hiding this comment.
[nitpick] The variable name 'mask' is reused to store a computed boolean mask, which can be confusing given that it is also a function parameter. Consider renaming the local variable (e.g., 'computed_mask') to improve clarity.
PR Review 🔍
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @aimura09 - I've reviewed your changes - here's some feedback:
- The nested loops applying the mask within
clash_error_compcould potentially be vectorized for improved clarity and performance. - Consider adding a type hint or docstring clarification for the expected format and shape of the
maskparameter insym_redock.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PR Code Suggestions ✨
|
- When this flag is on, sym_redock will be applied only on diffused residues and motifs will remain at the same xyz. Fix: lines suggested by Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a masking feature to the symmetry redocking process so that when enabled, only diffused residues are redocked while motifs remain unchanged. Key changes include:
- Adding a new optional mask parameter to the sym_redock function.
- Updating the clash error computation to account for masking both intra- and inter-residue interactions.
- Enhancing the looping logic to apply masking to specific contact regions.
| return allframes, frames | ||
|
|
||
| def sym_redock(xyz, Lasu, frames, opt, **_): | ||
| def sym_redock(xyz, Lasu, frames, opt, mask=None, **_): |
There was a problem hiding this comment.
Add a detailed docstring for the 'mask' parameter in sym_redock (and in clash_error_comp) that specifies the expected type (1D th.Tensor or list), shape, and intended behavior, to improve clarity and maintainability.
| dsymm = th.cdist(Xsymmall, Xsymmall, p=2) | ||
| dsymm_2 = dsymm.clone() | ||
| if mask is not None: | ||
| mask0 = mask[:Lasu.item()] if isinstance(Lasu, th.Tensor) else th.tensor(mask[:Lasu]) |
There was a problem hiding this comment.
[nitpick] Ensure that 'Lasu' is always provided as an integer value (or explicitly converted) since it is used both for slicing and for controlling loop iterations; consider renaming or converting it to clearly indicate its role as a count of residues.
User description
Description
maskparameter to thesym_redockfunction to allow selective application of symmetry redocking.Changes walkthrough 📝
sym_util.py
Enhance sym_redock with masking functionalityipd/sym/sym_util.py
maskparameter to thesym_redockfunction.clash_error_compfunction to handle the newmaskparameter.
provided mask.