Pass extrapolate to constructor

[sweemer]

Currently extrapolation can only be enabled using the Extrapolator::enableExtrapolation method, but it's useful to be able to enable it when constructing the extrapolator for the following reasons:

1. It allows the extrapolator to be const once constructed.
2. It works with prvalues, not just with lvalues.

In order to keep this PR relatively small, there are some other subclasses of Extrapolator that I haven't yet changed, but I may create another PR to change those later.

By the way, I notice that extrapolate is passed as a function argument to some functions like checkRange and checkStrike, but as far as I can tell there is nothing that enforces the value to be the same as Extrapolator::extrapolate_. Is there a situation in which they can legitimately be different? If not, then how about we deprecate the extrapolate function argument and use the value of the member variable directly?

[coveralls]

coverage: 72.642%. remained the same
when pulling 6a63c58 on sweemer:enable-extrapolation
into 0e83ab7 on lballabio:master.

[lballabio]

By the way, I notice that extrapolate is passed as a function argument to some functions like checkRange and checkStrike, but as far as I can tell there is nothing that enforces the value to be the same as Extrapolator::extrapolate_.

That's because passing the function argument is a way to override the setting only for that particular call.

[lballabio]

The disadvantage of this PR is that we're writing the default in a lot of places. I've seen forked codebases in which extrapolation is set as the default, and currently this can be done by flipping the default in Extrapolator. With this PR, it would require changing N files.

[sweemer]

By the way, I notice that extrapolate is passed as a function argument to some functions like checkRange and checkStrike, but as far as I can tell there is nothing that enforces the value to be the same as Extrapolator::extrapolate_.

That's because passing the function argument is a way to override the setting only for that particular call.

Hmm, as mentioned I only see the extrapolate argument being passed to the check functions and not to the interpolate functions themselves. Maybe I'm looking in the wrong place but could you point out an example of where the extrapolate argument is overriding the value in the extrapolator?

[sweemer]

The disadvantage of this PR is that we're writing the default in a lot of places. I've seen forked codebases in which extrapolation is set as the default, and currently this can be done by flipping the default in Extrapolator. With this PR, it would require changing N files.

I count 97 places where the value is defaulted on the current master branch, but I understand the point. Perhaps we should find a better way to make this configurable so that forking is not required? I still think the two benefits I pointed out in the description are worth pursuing if possible.

[lballabio]

could you point out an example of where the extrapolate argument is overriding the value in the extrapolator?

In checkRange itself (https://github.com/lballabio/QuantLib/blob/v1.35/ql/termstructure.cpp#L59) but I see it's one sided: passing extrapolate=true when the default is false will skip the check (and therefore extrapolate) but passing extrapolate=false when the default is true will still extrapolate. I guess the latter is not really a use case, since nobody complained so far...

[lballabio]

I count 97 places where the value is defaulted on the current master branch

Ouch. Can you point out a few? Changing the default in Extrapolator works, at least for term structures.

Perhaps we should find a better way to make this configurable so that forking is not required?

Yes, I thought about this while looking at this PR, but having a global switch would probably go in the direction of keeping the enableExtrapolation/disableExtrapolation methods. With a runtime global default, passing the argument to the constructors becomes more tricky because at that point we need three states: enable, disable, use the default. That calls for an optional<bool> where nullopt models "use the default", but I don't think the added complexity (here and in the SWIG wrappers) is worth the work.

I still think the two benefits I pointed out in the description are worth pursuing if possible.

Mind you, we wouldn't really get the first one. All term structures are passed around and stored as shared_ptr<YieldTermStructure> (I'm using yield as an example, but it's the same for other term structures) and those contain a pointer to non-const. Switching to shared_ptr<const YieldTermStructure> is simply not doable.

[sweemer]

Can you point out a few?

For example, YieldTermStructure::discount and all the other member functions that can be used to override the value for a particular call.

All term structures are passed around and stored as shared_ptr<YieldTermStructure>

That's true for term structures, but other subclasses of Extrapolator such as Interpolation can be used as standalone utilities. As a compromise, how about I remove the constructor arg from all of the term structures, and just keep it in the interpolators?

[lballabio]

For example, YieldTermStructure::discount

Oh, ok, the overrides are defaulted to "don't override". At this point it seems like a lucky accident that the overrides only work when the default for the term structure is not to extrapolate...

other subclasses of Extrapolator such as Interpolation can be used as standalone utilities

Warning: Interpolation and their subclasses are very risky to use standalone, because they don't store their data, only iterators into them, and so one needs to ensure that the lifetime of the data doesn't end before the interpolation is destroyed. The safe way to do this is for the interpolation to be a data member of another class, together with the data. But in that scenario you don't want the interpolation to be const, because when the data change you'll generally need to call update to recalculate the interpolation coefficients. And if it's a data member, constness is better driven by the containing object anyway.

I guess my actual question, though, is whether this is a worthwhile use of your time? The effort seems non trivial (also considering the three-states issue I mentioned) and the gain, well, not a lot. It doesn't even simplify the code.

[sweemer]

Warning: Interpolation and their subclasses are very risky to use standalone, because they don't store their data,

Yes, I understand that Interpolation is non-owning, and also that update must be non-const. Believe it or not, I actually do have a use case in my codebase where a subclass of Interpolation could be made const if only I could pass the extrapolation argument to the constructor.

I guess my actual question, though, is whether this is a worthwhile use of your time? The effort seems non trivial (also considering the three-states issue I mentioned) and the gain, well, not a lot. It doesn't even simplify the code.

It may not simplify the library code, but I think that my change does simplify user code, because the extra line to call enableExtrapolation can be inlined into the call to the constructor. I would also argue that it is both conceptually and aesthetically nicer to be able to initialize member variables when constructing the object and not after the object has already been constructed. Regarding the three-states issue, I don't recall being constrained by other people's forks in the past. In general I would expect the burden of maintaining a fork to fall on the one who forked, not on the maintainers of the upstream library.

Anyway as always the final decision is yours. I recognize that this PR is not as straightforward as I originally thought it would be. I will close it if you still think the cost outweighs the benefit.

[lballabio]

Hmm, I don't know. It might just be that I'm turning into the guy on the right.

But I'd rather skip this one. Also because depending on the curve, I'd rather think whether we can specify how to extrapolate instead, and this might get in the way.

[sweemer]

Don't worry, you're not that guy yet :) And no worries, let's skip this one.