Skip to content
This repository was archived by the owner on Mar 28, 2024. It is now read-only.

Conversation

@borisdavid
Copy link
Contributor

Context

While comparing daycount and elgorian, I noticed an inconsistency on daycount side concerning ActualActualAFB which produced a wrong value when to was before from and a 29th February was comprise within the interval [to, from].

Content

Since the year fraction computation is done on a backward induction, the function now inverts from and to if to is before from.

A property test is added to make sure that inverting from and to returns the opposite value (* -1.0).

@borisdavid borisdavid added the bug Something isn't working label Jan 11, 2024
@borisdavid borisdavid self-assigned this Jan 11, 2024
@borisdavid borisdavid requested a review from a team as a code owner January 11, 2024 17:14
olivierjacob
olivierjacob previously approved these changes Jan 12, 2024
daycount.go Outdated
alpha = -1.0

remaining = from
from, to = to, remaining
Copy link

@RemiMattheyDoret RemiMattheyDoret Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny detail. I think it makes the swap a bit more obvious if it is written as

Suggested change
from, to = to, remaining
from, to = to, from

daycount.go Outdated

// This function uses backward induction from the later date,
// 'from' and 'to' are swapped if 'to' falls before 'from'.
alpha := 1.0
Copy link

@RemiMattheyDoret RemiMattheyDoret Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would sign, directionality, dateOrdering, dateOrderingMultiplier, isAfter, fromAfterTo, isFromAfterTo be any better than alpha? (alpha might be just fine, I'm just sending in ideas).

Sadly, in Go, we cannot multiply by a bool (and we cannot simply cast a bool into an int), as alpha, really, should be a bool. Of course, we could do

sign := true

asInt := func(b bool) int {
    if b {
         return 1
    }
    
    return -1
}

return asInt(sign) * (float64(nbFullYears) + float64(remaining.Sub(from))/computeYearDurationAFB(from, remaining))

but that might be considered as over-engineering for such a small thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked myself the same question :)
It's generally called signum (at least in Java SDK)

Copy link

@RemiMattheyDoret RemiMattheyDoret Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like signum!

It is pronounced sig num (with the gueh sound), not sign um (with the nieh sound), right? 😅

@borisdavid borisdavid merged commit d20ca1a into main Jan 12, 2024
@borisdavid borisdavid deleted the code-inverted-yf-opposite-sign branch January 12, 2024 10:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants