Use a 365.25 denominator for the JULIAN day count - #253
Open
dylanpulver wants to merge 1 commit into
Open
Conversation
The JULIAN member documents itself as "actual divided by 365.25 as in Julian Calendar", but year_frac divides by 365, which makes it return exactly the same accrual factor as ACT_365F. The rename from SIMPLE to JULIAN inlined the old G_DAYS_IN_YEAR (365) constant rather than the 365.25 the new name and comment call for; the unit test kept the pre-rename expected value, so it pinned 365. Update the test to the value implied by the definition: 1 Jan 2019 to 21 May 2019 is 140 days, and 140 / 365.25 = 0.38330.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DayCountTypes.JULIANdocuments itself atday_count.py:60as "actual divided by 365.25 as in Julian Calendar", butyear_fracdivides by 365 atday_count.py:284. A Julian year is 365.25 days by definition, so the member as shipped returns exactly the same accrual factor asACT_365Fand carries no distinct meaning.On current master, 1 Jan 2019 -> 21 May 2019:
after:
JULIAN acc_factor=0.38329911019849416 den=365.25.How this happened (
310bab06, "Significant edits to improve DiscountCurve..."): that commit renamedSIMPLE->JULIAN, rewrote the comment to say 365.25, and replacedden = G_DAYS_IN_YEARwith a hardcodedden = 365.0— the old constant's value rather than the new definition's. It also repointed the test atDayCountTypes.ASTRONOMICAL, which does not exist;3c1c7f0crepaired the test toJULIANtwo days later but left the pre-rename expected value, so the test has been pinning 365 ever since.G_DAYS_IN_YEARis still imported atday_count.py:12and is now otherwise unused, which is the leftover of that inline.Change:
den = 365.25, and the unit test's expected value re-derived from the definition rather than from the implementation — 140 days / 365.25 = 0.38330 — plus an assertion thatJULIANdoes not collapse ontoACT_365F.Measured, same command and venv (Python 3.13,
pytest unit_tests, which is whattestpathsgives CI):8ba5e2c0: 957 passedTwo mutants, to show the test is not vacuous:
day_count.pyonly, keep the new test ->assert 0.3836 == 0.3833, 1 failed / 9 passedden = G_DAYS_IN_YEAR(restoring the now-dead import) -> same failure, which is the point: 365 in any spelling is not the Julian yearHow I found it: not from real FinancePy usage. I was systematically reading day-count convention tables across quant libraries looking for branches that are byte-identical to a sibling while their own comment says otherwise.
JULIANmatched, and the git history confirmed the intent rather than my guess at it.Not tested:
regression_tests/(not intestpaths) — butgrep -rn JULIANfinds no use of this member anywhere infinancepy/outside its own definition, so no internal pricing path changes. If you intendedJULIANto stay a 365-denominator alias, the right fix is the comment instead and I'm happy to flip it.This was written with AI assistance (Claude).