Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose an IBOR coupon's decision about whether it has fixed or not #1781

Merged

Conversation

tomwhoiscontrary
Copy link
Contributor

It is sometimes useful to look at an IBOR coupon and know whether it is based on a historical fixing or a forecast. This change exposes that decision, and also - IMHO! - cleans up the code to get a fixing a little.

This change does mean that for fixings today, we now call Index::pastFixing twice. Personally i don't think that's a big deal (especially since #1731). Otherwise, i think the functional and non-functional behaviour is the same.

I think this method would probably make sense in FloatingRateCoupon, using the same logic. I could pull it up if you think that would be useful. However, in that case, we should think about what OvernightIndexedCoupon.

} else {
// fixingDate_ == refDate
if (QuantLib::Settings::instance().enforcesTodaysHistoricFixings()) {
return true;
Copy link
Owner

Choose a reason for hiding this comment

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

This no longer holds once you add the refDate argument to the method call. enforcesTodaysHistoricFixings only comes into play when refDate is the evaluation date.

@tomwhoiscontrary
Copy link
Contributor Author

Have changed things around, so the test for enforcesTodaysHistoricFixings is only applied if the refDate is the evaluation date.

}
}
} else {
return false;
Copy link
Owner

Choose a reason for hiding this comment

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

Here fixingDate == refDate but refDate != evaluationDate. We should still check if there is a past fixing loaded.

I think we need a series of tests for this.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not entirely sure of what we should do in this particular case, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the tests go in QuantLib/test-suite/cashflows.cpp? Or a new QuantLib/test-suite/iborcoupon.cpp?

Copy link
Owner

Choose a reason for hiding this comment

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

cashflows.cpp is ok.

Copy link
Owner

Choose a reason for hiding this comment

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

Hi Tom, it seems we stalled on this. Do you have time to have another look? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would definitely like too. My current QuantLib priority is solving the mystery of the performance regression i see, which blocks any further upgrade. Once that's done i would like to get back to this, so we can stop carrying this patch. But of course, there will be other work to do at some point.

Copy link
Contributor

This PR was automatically marked as stale because it has been open 60 days with no activity. Remove stale label or comment, or this will be closed in two weeks.

@github-actions github-actions bot added the stale label Nov 22, 2023
@lballabio lballabio removed the stale label Nov 22, 2023
Copy link
Contributor

This PR was automatically marked as stale because it has been open 60 days with no activity. Remove stale label or comment, or this will be closed in two weeks.

@github-actions github-actions bot added the stale label Jan 22, 2024
@lballabio lballabio removed the stale label Jan 22, 2024
Copy link
Contributor

This PR was automatically marked as stale because it has been open 60 days with no activity. Remove stale label or comment, or this will be closed in two weeks.

@github-actions github-actions bot added the stale label Mar 23, 2024
@lballabio lballabio removed the stale label Mar 29, 2024
Copy link
Contributor

This PR was automatically marked as stale because it has been open 60 days with no activity. Remove stale label or comment, or this will be closed in two weeks.

@coveralls
Copy link

Coverage Status

coverage: 72.559% (+0.009%) from 72.55%
when pulling f9c5f08 on tomwhoiscontrary:pr-expose-ibor-coupon-has-fixed
into 8770770 on lballabio:master.

@coveralls
Copy link

Coverage Status

coverage: 72.558% (+0.008%) from 72.55%
when pulling f9c5f08 on tomwhoiscontrary:pr-expose-ibor-coupon-has-fixed
into 8770770 on lballabio:master.

@tomwhoiscontrary
Copy link
Contributor Author

I started again from scratch, with tests this time. The implementation is similar but not identical.

This version does not take a reference date (yet). I'm still thinking about that.

@tomwhoiscontrary
Copy link
Contributor Author

Apparently one build hasn't finished yet:

build (Clang 16, clang16, mantic, clang, clang++, true) Expected — Waiting for status to be reported

Which by now presumably means it's dead or stalled.

@lballabio lballabio modified the milestone: Release 1.35 Jun 24, 2024
@coveralls
Copy link

Coverage Status

coverage: 72.578% (+0.03%) from 72.55%
when pulling 3eac73c on tomwhoiscontrary:pr-expose-ibor-coupon-has-fixed
into 8770770 on lballabio:master.

@lballabio lballabio merged commit 37c84ff into lballabio:master Jun 24, 2024
41 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.

3 participants