Skip to content

Substitute the nominal tail per element rather than for the whole array - #1428

Merged
henrydingliu merged 3 commits into
casact:mainfrom
Abhayindia:chore/1414-tailcurve-warnings
Sep 26, 2026
Merged

henrydingliu merged 3 commits into
casact:mainfrom
Abhayindia:chore/1414-tailcurve-warnings

Conversation

@Abhayindia

@Abhayindia Abhayindia commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Refers #1414.

What is happening

TailBase._get_tail_weighted_time_period guards the logarithm with

tail = tail if tail.max() > 1 else 1.001

That 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.

TailBondy on the grouped clrd sample is the case in the test suite:

tails computed 12
tails at or below 1 6 (0.385901, 0.682751, 0.692166, 0.733976, 0.823642, 0.864306)
largest tail 1.018114
guard tail.max() > 1 True, so 1.001 is not substituted

Half the array evaluates the logarithm of a negative number. xp.where applies the same nominal 1.001 per element, which is what the surrounding code already intends.

This removes the 24 invalid value encountered in log warnings raised by test_workflow.py.

Also in here

The WeightedRegression fit two lines above tail was computed, discarded, and recomputed identically straight afterwards. Removed the first one.

Effect on results

Comparing tail_, sigma_, std_err_, ldf_ and cdf_ across raa, quarterly, tail_sample and clrd for TailCurve, TailConstant and TailBondy, 50 of 52 fingerprints are unchanged. The two that move are clrd TailCurve sigma_ and std_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 of log(0).

Scope

Per @henrydingliu this addresses the symptom rather than the intent of #1414, which is for reg_threshold and 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 clrd 24 of 775 tails come back as inf because the xp.prod of the last few LDFs overflows. log(inf - 1) is inf, so exp overflows downstream, and sigma_ * 0 then gives inf * 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_one covers the mixed case. Confirmed it fails on main without the change.

Full suite: 1268 passed, 7 skipped. Ruff clean on both files.

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Pyright Type Completeness

View the full pyright --verifytypes output for this commit

Project (full chainladder package, at this PR's head): 15.5% of exported symbols fully typed (221 / 1428)

Known Ambiguous Unknown Total
Project (head) 221 108 1099 1428

Other symbols referenced but not exported by chainladder: 13

Known Ambiguous Unknown Total
Other (head) 3 1 9 13

Symbols without documentation:

  • Functions without docstring: 326
  • Functions without default param: 0
  • Classes without docstring: 8

Patch (exported symbols added or changed by this PR): 0.0% fully typed (0 / 2)

Known Ambiguous Unknown Total
Patch 0 0 2 2
Patch symbol details
Symbol Status Change
chainladder.tails.tests.test_exponential.test_no_log_warning_when_only_some_tails_exceed_one ❌ unknown new
chainladder.tails.tests.test_exponential.test_nominal_tail_option_is_honoured ❌ unknown new

@codecov

codecov Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.07%. Comparing base (2d03c3b) to head (84701a1).
⚠️ Report is 5 commits behind head on main.

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           
Flag Coverage Δ
unittests 94.07% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@henrydingliu

henrydingliu commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

tail = tail if tail.max() > 1 else 1.001 is clearly not doing something that's sensible in the context of the _get_tail_weighted_time_period method. so a fix here is eventually needed. but even though this PR addresses the symptom of #1414, it doesn't resolve the intent of #1414, which is to let reg_threshold and its existing default value catch negative log, before one ever gets to the tail.max snippet. please change the description to refers #1414 rather than closes.

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.

@henrydingliu henrydingliu self-assigned this Sep 24, 2026
@Abhayindia

Copy link
Copy Markdown
Contributor Author

Changed to refers, and promoted the constant to options.NOMINAL_TAIL (default 1.001) with a test that it is honoured. You were right that it should not be buried in a private method.
Worth flagging what I found measuring it: the substitution is inert almost everywhere, because the entries it rescues usually have NaN regression coefficients already. The only case in the suite where the value reaches an output is full clrd with TailCurve, at the single entry whose tail is exactly 1.0. Noted on #1414 that reg_threshold cannot be the guard here.

@henrydingliu

Copy link
Copy Markdown
Member

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.

@Abhayindia

Copy link
Copy Markdown
Contributor Author

Both tests now take the clrd fixture. That also runs them on the sparse backend, and I checked they still fail without the library change on both: 4 failures across the two tests times the two parametrisations.
Full suite 1271 passed, ruff clean.

@henrydingliu
henrydingliu merged commit fe4d11c into casact:main Sep 26, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants