Consistently use Real datatype #1400

auto-differentiation-dev · 2022-06-08T15:24:34Z

This replaces many stray uses of the double datatype with QuantLib::Real, to ensure it is used consistently throughout the library and test suite.

In particular:

it replaces double with Real
it initialises accumulators in std::inner_product and std::accumulate with Real literals, to ensure the values are aggregated with the correct data type internally
it avoids constexpr Real x = ... statements, as with the global typedef we cannot generally assume that Real can in fact be initialised in a constexpr

boring-cyborg · 2022-06-08T15:24:36Z

Thanks for opening this pull request! It might take a while before we look at it, so don't worry if there seems to be no feedback. We'll get to it.

CLAassistant · 2022-06-08T15:24:51Z

All committers have signed the CLA.

lballabio · 2022-06-08T16:24:47Z

ql/experimental/exoticoptions/analyticholderextensibleoptionengine.cpp

@@ -65,7 +65,7 @@ namespace QuantLib {
        //calculate payoff discount factor assuming continuous compounding
        DiscountFactor discount = riskFreeDiscount(t1);
        Real result = 0;
-        constexpr Real minusInf = -std::numeric_limits<Real>::infinity();
+        const Real minusInf = -std::numeric_limits<Real>::infinity();


These and the others are constants, so I'd use double as you did in the change right above. What do you think?

We thought about that, but there could be an issue with this in the remote (admittedly unlikely) case that Real is something else than double with a different representation of infinity. If we define a constant with double-infinity, and then use it in calculations with Real, it may become an issue.

The case above is different, as it's just a regular number, not infinity.

I'd keep the double constants now and think about the unlikely case if it comes up.

Ok, that is just pushed and fixed as suggested.

lballabio · 2022-06-08T18:23:25Z

ql/interestrate.hpp

@@ -185,6 +185,15 @@ namespace QuantLib {
        Real freq_;
    };

+    // helper to avoid relying on implicit conversion to Rate
+    inline Rate operator-(const InterestRate& a, const InterestRate& b) {


Hmm, I didn't realize one could add or subtract InterestRate instances. I would probably remove these operators and write an explicit call to rate at the point of use, to avoid adding or subtracting rates with different conventions.

We tend to agree on that point. However, in that case I would remove the implicit conversion operator in the InterestRate class to Rate (i.e. Real) altogether. Otherwise such operations can be added without developers noticing.

One example where this is used is in ql/experimental/processes/extendedblackscholesprocess.cpp, but there are 15 uses in total throughout the library.

What do you think?

I don't disagree, but removing the implicit conversion would break backward compatibility.

Removing these operators at least would help us find and avoid implicit conversions in subtractions and additions, which are more at risk of inconsistency because they involve two different rates.

That's just pushed - we changed it at the point of use to an explicit call to rate and removed those operators.

coveralls · 2022-06-08T19:32:40Z

Coverage remained the same at 71.145% when pulling ad19ccc on xcelerit:feature/consistent-real into 256cc86 on lballabio:master.

github-advanced-security

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

Examples/BasketLosses/BasketLosses.cpp

sweemer · 2022-06-09T02:03:44Z

it avoids constexpr Real x = ... statements, as with the global typedef we cannot generally assume that Real can in fact be initialised in a constexpr

As mentioned by Luigi, constexpr local variables can be kept as doubles. But in principle the type defined as QL_REAL should support constexpr initialization because other defines such as QL_MIN_REAL and QL_EPSILON are defined in terms of QL_REAL using constexpr functions from std::numeric_limits. If there are no compile errors yet then it probably means that these defines are not used themselves in a constexpr context, but it doesn't necessarily mean that they won't be at some point in the future.

sweemer · 2022-06-09T02:15:21Z

Also, I think you will need to add a CI job to pass in the specific type of Real that you care about to make sure that there are no compilation errors after any future commits.

lballabio · 2022-06-09T09:28:47Z

other defines such as QL_MIN_REAL and QL_EPSILON are defined in terms of QL_REAL using constexpr functions from std::numeric_limits.

Using std::numeric_limits with the new type would require writing a new specialization, and the methods defined there might not be constexpr, I guess?

sweemer · 2022-06-09T11:02:15Z

Using std::numeric_limits with the new type would require writing a new specialization, and the methods defined there might not be constexpr, I guess?

Right, so if somebody later makes a change to the library to use QL_EPSILON at compile-time then the non-constexpr specialization will no longer be good enough.

I think that preserving the ability to use the QL_* values at compile time with QL_REAL typedef'ed to double outweighs the obligation (if one even exists) to support any possible typedef of QL_REAL. So in my opinion the burden is on the user to provide constexpr specializations of std::numeric_limits<Real>::... if the library requires it at any point in the future.

auto-differentiation-dev · 2022-06-09T13:05:30Z

Also, I think you will need to add a CI job to pass in the specific type of Real that you care about to make sure that there are no compilation errors after any future commits.

This is a good suggestion indeed @sweemer - that would prevent any stray use of double moving forward. We're open to do that with a standalone Real replacement type for testing purposes, but that does not need to be part of this PR. We can do that at a later stage.

auto-differentiation-dev · 2022-06-09T13:10:31Z

Regarding the constexpr discussion:

In general, a Real typedef to something other than a built-in floating point type (float or double) typically means that constexpr intialisations may not work, i.e. compile-time constants for QL_EPSILON etc are not possible.

However, we suggest when/if this issue comes up, QL_EPSILON etc may be defined as an "underlying" built-in floating point type, where constexpr works. For example, if a struct is used which internally wraps double, QL_EPSILON etc may be defined in terms of double (not Real). And a specialisation of std::numeric_limits<Real> needs to be provided that returns double. This is definitely possible, wouldn't break semantics, and keeps compile-time logic working.

However, we think for this PR no further changes are needed and we can keep the constants as double as per Luigi's suggestion. The above suggestion could be put in place when it is needed.

boring-cyborg · 2022-06-13T18:19:20Z

Congratulations on your first merged pull request!

consistently use Real datatype

7067ec8

auto-differentiation-dev marked this pull request as ready for review June 8, 2022 15:25

lballabio reviewed Jun 8, 2022

View reviewed changes

github-advanced-security bot found potential problems Jun 8, 2022

View reviewed changes

sweemer reviewed Jun 9, 2022

View reviewed changes

Examples/BasketLosses/BasketLosses.cpp Show resolved Hide resolved

auto-differentiation-dev added 3 commits June 9, 2022 11:25

Adding previously-missed double->Real replacements

5c9ec21

changing constants to double constexpr rather than Real

297d373

avoid adding/subtracting interest rates, call rate() method instead

cadd867

Adding Real initialiser to inner_product

ad19ccc

lballabio added this to the Release 1.27 milestone Jun 13, 2022

lballabio merged commit 42dc7d0 into lballabio:master Jun 13, 2022

auto-differentiation-dev mentioned this pull request Jun 24, 2022

Further changes for consistent Real usage #1413

Merged

xcelerit-team deleted the feature/consistent-real branch November 23, 2022 09:41

auto-differentiation-dev mentioned this pull request Jul 29, 2024

Failed to build with Quantlib CMAKE_CXX_STANDARD set to 20 auto-differentiation/QuantLib-Risks-Cpp#18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistently use Real datatype #1400

Consistently use Real datatype #1400

auto-differentiation-dev commented Jun 8, 2022

boring-cyborg bot commented Jun 8, 2022

CLAassistant commented Jun 8, 2022 •

edited

Loading

lballabio Jun 8, 2022

auto-differentiation-dev Jun 9, 2022

lballabio Jun 9, 2022

auto-differentiation-dev Jun 9, 2022

lballabio Jun 8, 2022

auto-differentiation-dev Jun 9, 2022

lballabio Jun 9, 2022

auto-differentiation-dev Jun 9, 2022

coveralls commented Jun 8, 2022 •

edited

Loading

github-advanced-security bot left a comment

sweemer commented Jun 9, 2022

sweemer commented Jun 9, 2022

lballabio commented Jun 9, 2022

sweemer commented Jun 9, 2022

auto-differentiation-dev commented Jun 9, 2022

auto-differentiation-dev commented Jun 9, 2022

boring-cyborg bot commented Jun 13, 2022

Consistently use Real datatype #1400

Consistently use Real datatype #1400

Conversation

auto-differentiation-dev commented Jun 8, 2022

boring-cyborg bot commented Jun 8, 2022

CLAassistant commented Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 8, 2022 • edited Loading

github-advanced-security bot left a comment

Choose a reason for hiding this comment

sweemer commented Jun 9, 2022

sweemer commented Jun 9, 2022

lballabio commented Jun 9, 2022

sweemer commented Jun 9, 2022

auto-differentiation-dev commented Jun 9, 2022

auto-differentiation-dev commented Jun 9, 2022

boring-cyborg bot commented Jun 13, 2022

CLAassistant commented Jun 8, 2022 •

edited

Loading

coveralls commented Jun 8, 2022 •

edited

Loading