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

Allow constraints in FittedBondDiscountCurve::FittingMethod (#1954) #2059

Merged
merged 5 commits into from
Aug 20, 2024

Conversation

klin333
Copy link
Contributor

@klin333 klin333 commented Aug 19, 2024

fulfills feature request in #1954

@CLAassistant
Copy link

CLAassistant commented Aug 19, 2024

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Aug 19, 2024

Coverage Status

coverage: 72.67% (+0.02%) from 72.651%
when pulling ce726f4 on klin333:fitting_constraints
into c0fac24 on lballabio:master.

@lballabio
Copy link
Owner

Thanks! I was just about to ask you to turn your changes into a PR but you were faster :)

A couple of things I would modify:

  • you can use Constraint instead of ext::shared_ptr<Constraint>; the class itself is not polymorphic, only the internal Impl is;
  • the additional parameter can be also added to the constructors of the existing fitting methods and passed down to the base class constructor.

Are you comfortable making the changes? Otherwise give me a shout and I will add them to your PR.

@klin333
Copy link
Contributor Author

klin333 commented Aug 19, 2024

Thanks Luigi. It'll be really helpful if you can make those changes. I'm not sure if I understand what needs to be done. C++ is not native to me, I'm not comfortable in getting all the const & std::move etc right.

I do agree with your second point. In fact, it was the desire to have constraints on Nelson Siegel that drove this feature request in the first place. Only reason why I didn't add constraint argument to the existing fitting methods was to keep this PR small.

@klin333
Copy link
Contributor Author

klin333 commented Aug 19, 2024

Arh, also a minor fix to the test message required. If it's ok it might be good for you to help fix that too while you are at it, so that I don't pollute the commit log with too many commits.

existing commit: BOOST_TEST_MESSAGE("Testing that fitted bond curves check the guess size when given...");
pls change it to: BOOST_TEST_MESSAGE("Testing that fitted bond curves respect contraints...");

@lballabio lballabio added this to the Release 1.36 milestone Aug 20, 2024
@lballabio lballabio linked an issue Aug 20, 2024 that may be closed by this pull request
@lballabio lballabio merged commit fc4210f into lballabio:master Aug 20, 2024
42 checks passed
Copy link

boring-cyborg bot commented Aug 20, 2024

Congratulations on your first merged pull request!

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.

FittedBondDiscountCurve: allow constraints
4 participants