Conversation
…message/update layers, documentation
There was a problem hiding this comment.
Pull request overview
This PR extends the flow model to optionally consume encoder-produced protein–protein edge features through the GVP message passing stack, while also refactoring training to support external optimizer stepping / gradient accumulation and improving GVP module configurability + documentation.
Changes:
- Add support for passing
pp_edge_attrfrom the encoder intoProteinWaterUpdate/GVPMultiEdgeConvand make GVP depth configurable vian_message_gvps/n_update_gvps. - Refactor
FlowMatcher.training_stepto perform forward+backward only (caller performsoptimizer.step()), with anaccumulation_stepsparameter. - Introduce constants (
NUM_RBF,RBF_CUTOFF) usage in GVP/flow modules and expand docstrings/formatting; update tests accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
tests/test_flow.py |
Updates mocks for the new encoder return shape and adds a forward test intended for an ESM-based encoder. |
src/gvp.py |
Adds documentation and adds optional cached edge attribute plumbing to heterogeneous GVP message passing; switches some defaults to constants. |
src/flow.py |
Threads encoder-provided PP edge features into the updater, adds configurable GVP depths, changes the vector field head dims, and refactors training_step for accumulation/caller-managed optimizer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
marcuscollins
left a comment
There was a problem hiding this comment.
I think these PRs need to be reorganized so that they don't break main. There are too many things here where it is hard to evaluate what's happening because they require changes in other places not included in this PR.
I've put comments in here, so have a look, but try to isolate each functional change, like adding an argument to a method's signature, and make separate PRs for those. Make a separate PR for changes that are just formatting.
marcuscollins
left a comment
There was a problem hiding this comment.
You get rid of some code in this PR (gradient clipping, stepping the optimizer) that worries me. I don't see where the optimizer.step() method is called, which can't be right, unless it was previously called in multiple places and you're removing one. Otherwise mostly requests for clarification.
| ) | ||
| optimizer.step() | ||
| # backward (scale loss for gradient accumulation) | ||
| (loss / accumulation_steps).backward() |
There was a problem hiding this comment.
why get rid of gradient clipping? And optimizer.step()? This feels like something isn't right, since I don't see that step taken anywhere else.
There was a problem hiding this comment.
I've kept only the forward pass and loss computation in the training step here, and moved the grad clipping to the training script -- this is primarily because it makes gradient accumulation easier to control.
pp_edge_attr) in flow model, enabling GVP encoder to pass learned edge representations through the message passing layersn_message_gvpsandn_update_gvpsparameters to control GVP layer depthtraining_stepto separate gradient accumulation from optimizer step (caller now handles optimizer)NUM_RBF, RBF_CUTOFF) instead of hardcoded values throughout GVP module