Fix ADP restraint config being silently lost on target rebuild - #68
Fix ADP restraint config being silently lost on target rebuild#68kmdalton wants to merge 1 commit into
Conversation
`ADPSimilarityTarget.simu_sigma` is public API with a deliberate setter, but
there was no way to make a value survive. `Refinement._init_targets` builds
`TotalADPTarget(self.model, verbose=self.verbose)` passing no restraint
parameters, so every rebuild resets `simu_sigma` / `simu_sigma_aniso` to the
constructor defaults (2.0 / 1.0). `refine_rigid_body` rebuilds once per
resolution cutoff via `_rebind_for_data` -> `_init_targets`; the ensemble and
`create_from_state_dict` paths rebuild too.
The result: set a sigma, run rigid body, and refinement silently proceeds at
the default. No warning. There was no supported alternative -- no constructor
argument, no CLI flag (`--sigma-a-max` is sigma_A, a different quantity) -- so
post-construction assignment was the only way in, and it was exactly what got
discarded.
This is the failure mode already documented on `_xray_target_kwargs`: "a second
build site silently reverts whatever it forgets to pass, which once made five
CLI flags no-ops." The x-ray targets were given a single source of truth for
their construction kwargs; the ADP targets never were. This applies the same
pattern.
- `CombinedModelTargets` takes `component_config`, `{component: {kwarg: value}}`,
set before `_create_targets()` and exposed to subclasses via
`_component_kwargs()`. A component name that matches nothing raises rather
than no-op'ing, since a silent no-op is the bug being fixed. Config is deep
copied so a later mutation of the caller's dict cannot reach the target.
- `TotalADPTarget._create_targets` splats the per-component kwargs.
- `Refinement` takes `adp_restraints=...`, stores it alongside the other
pre-`_init_targets` configuration, and passes it on every rebuild with the
same `getattr` fallback `_xray_target_kwargs` uses for the ensemble and
state-dict paths.
Behaviour is unchanged when no config is passed.
LBFGSRefinement(..., adp_restraints={"simu": {"simu_sigma": 0.4}})
Verified against the reported scenario: post-construction assignment reads back
as 2.0 after `refine_rigid_body`, constructor config holds at 0.4 through rigid
body, a second `get_scales`, and `refine_adp`.
`TotalGeometryTarget` has the same latent issue -- its components are built
with no configuration path either -- but nothing sets geometry component
parameters today, so it is left alone. The base-class mechanism is generic, so
wiring it up later is two lines. A CLI flag for `--adp-restraints` would make
this reachable from `torchref.refine`; deliberately not bundled here.
Tests: tests/unit/test_adp_restraint_config.py, 7 cases covering defaults,
propagation, survival across a rebuild, copy-not-alias, and both misspelling
paths. Full unit + functional suite passes (1748 passed, 74 skipped), as do the
32 integration tests touching rigid body, ensemble, state-dict and CLI paths.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
hi @HatPdotS , i ran into an issue with the ADP target weights being reset to a default value between macrocycles. Claude proposes this fix. let me know if it sounds right to you. |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
one more detail -- i was using a custom refinement loop which runs rigid body during every macrocycle. if i understand correctly, that was the root cause. i can dig in more if needed. just wanted a second set of eyes on it. |
|
I moved the weights into the refinement loss_state object, and I think these should survive the rigid-body step. |
|
looking into it... |
ADPSimilarityTarget.simu_sigmais public API with a deliberate setter, but there was no way to make a value survive.Refinement._init_targetsbuildsTotalADPTarget(self.model, verbose=self.verbose)passing no restraint parameters, so every rebuild resetssimu_sigma/simu_sigma_anisoto the constructor defaults (2.0 / 1.0).refine_rigid_bodyrebuilds once per resolution cutoff via_rebind_for_data->_init_targets; the ensemble andcreate_from_state_dictpaths rebuild too.The result: set a sigma, run rigid body, and refinement silently proceeds at the default. No warning. There was no supported alternative -- no constructor argument, no CLI flag (
--sigma-a-maxis sigma_A, a different quantity) -- so post-construction assignment was the only way in, and it was exactly what got discarded.This is the failure mode already documented on
_xray_target_kwargs: "a second build site silently reverts whatever it forgets to pass, which once made five CLI flags no-ops." The x-ray targets were given a single source of truth for their construction kwargs; the ADP targets never were. This applies the same pattern.CombinedModelTargetstakescomponent_config,{component: {kwarg: value}}, set before_create_targets()and exposed to subclasses via_component_kwargs(). A component name that matches nothing raises rather than no-op'ing, since a silent no-op is the bug being fixed. Config is deep copied so a later mutation of the caller's dict cannot reach the target.TotalADPTarget._create_targetssplats the per-component kwargs.Refinementtakesadp_restraints=..., stores it alongside the other pre-_init_targetsconfiguration, and passes it on every rebuild with the samegetattrfallback_xray_target_kwargsuses for the ensemble and state-dict paths.Behaviour is unchanged when no config is passed.
Verified against the reported scenario: post-construction assignment reads back as 2.0 after
refine_rigid_body, constructor config holds at 0.4 through rigid body, a secondget_scales, andrefine_adp.TotalGeometryTargethas the same latent issue -- its components are built with no configuration path either -- but nothing sets geometry component parameters today, so it is left alone. The base-class mechanism is generic, so wiring it up later is two lines. A CLI flag for--adp-restraintswould make this reachable fromtorchref.refine; deliberately not bundled here.Tests: tests/unit/test_adp_restraint_config.py, 7 cases covering defaults, propagation, survival across a rebuild, copy-not-alias, and both misspelling paths. Full unit + functional suite passes (1748 passed, 74 skipped), as do the 32 integration tests touching rigid body, ensemble, state-dict and CLI paths.