Substitute the nominal tail per element rather than for the whole array - #1428
Conversation
Pyright Type CompletenessView the full Project (full
Other symbols referenced but not exported by
Symbols without documentation:
Patch (exported symbols added or changed by this PR): 0.0% fully typed (0 / 2)
Patch symbol details
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1428 +/- ##
=======================================
Coverage 94.06% 94.07%
=======================================
Files 96 96
Lines 5765 5768 +3
Branches 722 722
=======================================
+ Hits 5423 5426 +3
Misses 221 221
Partials 121 121
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
on the content of the PR itself, as long as we are touching the 1.001, can we formalize it as an option at the package level? let me know if you disagree but i don't think it's good practice to hide a hardcoded value affecting multiple modules inside a private method. |
|
Changed to refers, and promoted the constant to |
|
thanks for all the investigation! sorry i didn't catch this before. can you please update the tests to use the clrd test fixture? otherwise all looks good. |
|
Both tests now take the |
Refers #1414.
What is happening
TailBase._get_tail_weighted_time_periodguards the logarithm withThat is a whole-array test standing in for a per-element one. As soon as a single element exceeds 1, the substitution is skipped for every element, and any element at or below 1 reaches
log(tail - 1)with a non-positive argument.TailBondyon the groupedclrdsample is the case in the test suite:tail.max() > 1Half the array evaluates the logarithm of a negative number.
xp.whereapplies the same nominal 1.001 per element, which is what the surrounding code already intends.This removes the 24
invalid value encountered in logwarnings raised bytest_workflow.py.Also in here
The
WeightedRegressionfit two lines abovetailwas computed, discarded, and recomputed identically straight afterwards. Removed the first one.Effect on results
Comparing
tail_,sigma_,std_err_,ldf_andcdf_acrossraa,quarterly,tail_sampleandclrdforTailCurve,TailConstantandTailBondy, 50 of 52 fingerprints are unchanged. The two that move areclrdTailCurvesigma_andstd_err_, at the single index entry whose tail was exactly 1.0: it now takes the nominal 1.001 the guard was always meant to give it instead oflog(0).Scope
Per @henrydingliu this addresses the symptom rather than the intent of #1414, which is for
reg_thresholdand its default to catch the negative logarithm before execution ever reaches this guard. Changed to refers.Still open
Four warnings remain at lines 133, 137, 140 and 141, and they are a different cause: on
clrd24 of 775 tails come back asinfbecause thexp.prodof the last few LDFs overflows.log(inf - 1)isinf, soexpoverflows downstream, andsigma_ * 0then givesinf * 0. Clamping that changes results in a way I do not think should be decided without you, so I have left it. Happy to take it in a follow-up if you want it handled, and if so, what an infinite tail should resolve to.Verification
test_no_log_warning_when_only_some_tails_exceed_onecovers the mixed case. Confirmed it fails onmainwithout the change.Full suite: 1268 passed, 7 skipped. Ruff clean on both files.