Repository navigation
Refuse LogKDE(displace_t=True) for models whose RTs start below t - #375
Conversation
LogKDE(displace_t=True) shifts every RT by t and floors the labels of anything that lands at or below zero. For the models in KDE_NO_DISPLACE_T and those declaring ndt_edge_shift that puts the cutoff inside the response-time support: a uniform st kernel starts at t - st, and the Normal, truncated-Normal and Rayleigh kernels have no edge at t at all. TrainingDataGenerator already overrides displace_t to False for them, so the pipeline was never affected; a direct LogKDE caller got flat labels in the band below t without a word. LogKDE now resolves the model from metadata["model"] (a _deadline suffix stripped, as lan_mlp does) and raises a ValueError naming the model when its config declares ndt_edge_shift or its name is in KDE_NO_DISPLACE_T. An unregistered name is refused when the metadata carries a non-zero st. Every other model keeps the shift by t unchanged. This replaces #359 by EItanm1999, which recovered the boundary from the simulator metadata instead: t - st for the uniform kernels, which was right, and t + t_dist.keywords["loc"] otherwise, which for ddm_truncnormt is the kernel's centre rather than its edge at 0 and collapsed the labels. Since the generator never displaces these models, the honest behaviour for direct callers is an error pointing at displace_t=False. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every KDE_NO_DISPLACE_T model (plus full_ddm2, whose config name differs from its key) is refused at displace_t=True with the model named and still builds at displace_t=False. ddm, ddm_sdv and angle keep the plain shift by the metadata t, pinned against the label formula. An unregistered model is judged by its st, and a _deadline name is looked up by its base model. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughLogKDE now checks model metadata before shifting response times when ChangesDisplacement validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to LogKDE now raises a clear ValueError when displacement is requested for models whose response times can start below t. Other models keep the existing shift. No actionable merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation Issue Resolution Implement the
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Refuse
displace_t=Truefor models whose response times start belowtLogKDE(displace_t=True)shifts every RT bytand floors any label that lands at or below zero. For the models listed inKDE_NO_DISPLACE_T, and for those whose config declaresndt_edge_shift(#370), that puts the cutoff inside the response-time support: a uniformstkernel starts att - st, and the Normal, truncated-Normal and Rayleigh kernels have no edge attat all.TrainingDataGeneratoralready warns and overridesdisplace_ttoFalsefor these models, so the data pipeline was never affected, but a directLogKDEcaller got flat labels in the band belowtwith no warning.LogKDEnow resolves the model frommetadata["model"](a_deadlinesuffix stripped, aslan_mlpdoes) and raises aValueErrornaming the model and pointing atdisplace_t=Falsewhen its config declaresndt_edge_shiftor its config name is inKDE_NO_DISPLACE_T. An unregistered model name is refused when the metadata carries a non-zerost. Every other model keeps the shift byt, byte-for-byte.Credit
This replaces #359 by @EItanm1999, who found the problem and measured its effect on labels. #359 recovered the boundary from the simulator metadata (
t - stfor the uniform kernels,t + t_dist.keywords["loc"]otherwise). That is right for the uniform kernels but misreadsddm_truncnormt, wherelocis the kernel's centre and the support edge is 0, so its labels collapsed. Since the generator never displaces these models, direct callers get an explicit error instead of a different shift.Checks
tests/test_kde_class.py: 42 passed. New tests cover everyKDE_NO_DISPLACE_Tentry plusfull_ddm2(raises with the model named;displace_t=Falsestill works), the two guard clauses independently,ddm/ddm_sdv/anglelabels pinned to the shift-by-tformula, an unregistered model judged by itsst, and_deadlinenames. The 11 must-raise cases fail onorigin/mainwithout the source change; the 7 behaviour-pinning cases pass on both.LogKDE(displace_t=True)labels, samples and inputs forddm,ddm_sdv,angleevaluated on a fixed grid before and after the change: byte-identical.ruff checkandruff format --checkclean.git merge-treeagainstorigin/mainclean.Closes #359.
🤖 Generated with Claude Code
🤖 Generated with Claude Code
Summary by CodeRabbit