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

Reduce duplication in rate helpers constructors #1911

Merged

Conversation

eltoder
Copy link
Contributor

@eltoder eltoder commented Feb 15, 2024

No description provided.

@coveralls
Copy link

coveralls commented Feb 15, 2024

Coverage Status

coverage: 72.477% (+0.05%) from 72.425%
when pulling 30d6eb5 on eltoder:feature/swap-rate-helper-duplication
into cbec61e on lballabio:master.

@lballabio
Copy link
Owner

I would further remove duplication by having all the constructors taking a Rate quote delegate to the corresponding ones taking a Handle<Quote>. Using the base-class constructor taking a Rate only saves a registration to the handle, and I don't think it makes any difference. What do you think?

@eltoder
Copy link
Contributor Author

eltoder commented Feb 15, 2024

I thought of that, but wasn't sure if we want to keep the optimization of not registering to the handle. There are a few ways we can try to preserve it if we care. We could also use boost::variant<Rate, Handle<Quote>> as the first argument and remove the overloads completely. This will be some effort to support in SWIG, but we can also lie to SWIG and leave the overload declarations in .i files. I think that will work.

@lballabio
Copy link
Owner

I don't think the optimization is really an optimization, so I wouldn't put any effort into preserving it.

A variant looks way too much clever to me—if we want to reduce the overloads, we can deprecate the ones taking a double and add a convenience function like, for instance,

Handle<Quote> handle(Real q) { return Handle<Quote>(ext::make_shared<SimpleQuote>(q)); }

@eltoder
Copy link
Contributor Author

eltoder commented Feb 15, 2024

Variant is actually very easy. We typedef it to something like ValueOrQuote, add 2 lines in the base class to handle it, and just pass through in all derived classes without having to worry about how it works. All existing code will continue to work, but we have 2x fewer constructors.

@eltoder
Copy link
Contributor Author

eltoder commented Feb 15, 2024

I do agree with adding the convenience function. In one version of this PR I wrote it under static SimpleQuote::make() name.

@lballabio
Copy link
Owner

I don't doubt the variant is easy to add, but I'm not sure it's common vocabulary yet. Anyway—we can reduce duplication and keep the 2x constructors in this PR, and think about it later.

@eltoder
Copy link
Contributor Author

eltoder commented Feb 15, 2024

A related question: some of the duplication is because some constructors create an IborIndex from scratch while others clone an existing index. We can remove the duplication by making the former call the latter if we don't mind the cost of the extra clone. We may be able to avoid the clone with some if check.

@lballabio
Copy link
Owner

Ok for me. Again, if the instrument underlying the helper asks for an index, in the long run it's probably better for the helper to take an index, not the stuff to build an index, so we can deprecate the constructor taking the latter.

@eltoder eltoder force-pushed the feature/swap-rate-helper-duplication branch from 24309ab to cef64cc Compare February 16, 2024 01:07
@eltoder
Copy link
Contributor Author

eltoder commented Feb 16, 2024

@lballabio I left the indexes for now, but made the other changes.

@lballabio
Copy link
Owner

lballabio commented Feb 16, 2024

Thanks! I made a few more changes: I used the convenience method in a few examples, turned it into a method for brevity, and made it return a relinkable handle (not needed in the helpers, but possibly in other use cases).

@lballabio lballabio added this to the Release 1.34 milestone Feb 16, 2024
@eltoder
Copy link
Contributor Author

eltoder commented Feb 16, 2024

I think the name handle can make it harder for people to find where this function is defined or may seem too magical (can it turn anything into any handle?). Maybe quoteHandle or something more informative? I don't have a strong opinion, so up to you.

@eltoder
Copy link
Contributor Author

eltoder commented Feb 16, 2024

Another option is we can make an implicit conversion operator from SimpleQuote to Handle<Quote>. Then we can write SimpleQuote(rate) in all these cases. We'll need to add a && overload to make this work efficiently.

@lballabio
Copy link
Owner

Hmm, I didn't want it to be too long to write. Oh well, I'll let it rest over the weekend and come back with a fresh mind—hopefully.

@eltoder eltoder mentioned this pull request Feb 16, 2024
@lballabio
Copy link
Owner

I settled on makeQuoteHandle. Thanks!

@lballabio lballabio merged commit 72012a7 into lballabio:master Feb 19, 2024
48 checks passed
@eltoder eltoder deleted the feature/swap-rate-helper-duplication branch February 19, 2024 14:45
@eltoder
Copy link
Contributor Author

eltoder commented Feb 19, 2024

This makes sense to me.

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