simple_continued_fraction: added public functions to access and modify the coefficients

[Tomaqa]

• All coefficients are accessible and mutable, and the class can be used as a container.
• I removed partial_denominators, because IMO the name is misleading - front element represents integer part, and is not denominator. It may confuse the user that the function returns sth. where the integer part is omitted.
• I removed member variable x_ because I consider it useless - it is only necessary in the constructor, and also it would be difficult to make it consistent with the new mutable member function.
• The only exception are lines 154-155 (in the previous file), which I do not understand, e.g. how it uses .x_.backend(). But if the precision is somehow needed, only precision should be stored, not the original value.

Partially solves #970, but I did not handle documentation.

I also implemented conversion (non-member) function to boost::rational, but I am not sure where to place it, because it needs to include both simple_continued_fraction.hpp and rational.hpp.

[NAThompson]

All coefficients are accessible and mutable, and the class can be used as a container.

Why make the coefficients mutable?

[Tomaqa]

Mutability simply allows further operations with continued fractions, not only constructing them from floats.
For example, rational approximation requires to perform binary operations on already built continued fractions, not with floats.
Only const accessors to the coefficients are not enough in cases when a continued fraction is supposed to be a result of a binary operation of two continued fractions.
I believe that there are many more use cases than the mentioned approximation.

Modifying particular coefficients should not turn the object into an inconsistent state.
However, it is true that one can set a coefficient a_i, i > 0, to a non-positive number, which would result in an invalid simple continued fraction.
Also, I realized that also this thing should be taken care of, to keep the objects in the chosen "canonical form":

// Deal with non-uniqueness of continued fractions: [a0; a1, ..., an, 1] = a0; a1, ..., an + 1].
// The shorter representation is considered the canonical representation

Consistency can be easily preserved within push_back, but mutable operator [] may be replaced by a setter.

If these comments are OK with you, I can update the pull request ...

[Tomaqa]

Also, I can update the code in order to fix some of the tests that failed. But not all of them, for example in cases where partial_denominators are missing, which I argued that should be removed or renamed.

[NAThompson]

I removed partial_denominators, because IMO the name is misleading - front element represents integer part, and is not denominator. It may confuse the user that the function returns sth. where the integer part is omitted.

The current usage is consistent with Mathworld's definition. Are there more authoritative sources which exclude the integer part from the definition?

Only const accessors to the coefficients are not enough in cases when a continued fraction is supposed to be a result of a binary operation of two continued fractions.

The goal of this class is to convert a floating point number into its best continued fraction approximation. Therefore, unless we change the purpose of the class, it does not make sense to allow it to be mutable.

[Tomaqa]

The current usage is consistent with Mathworld's definition. Are there more authoritative sources which exclude the integer part from the definition?

I only found sources where either "Mathworld's definition" is used, or the coefficients b_i have no specific name.
So in the end, I suppose that the current form can be kept, I just added a note with clarification that it also includes the integer part.
Note that the Mathworld's definition contains several mistakes. If it is referenced somewhere in documentation, I would prefer this Mathworld's definition of continued fractions in general, where simple cf. are used as a special case, and the formula and the definition does not contain mistakes ...

The goal of this class is to convert a floating point number into its best continued fraction approximation. Therefore, unless we change the purpose of the class, it does not make sense to allow it to be mutable.

I believe that this is enough for many use cases, but definitely not for all use cases, and it would be pitty if one would have to implement brand new class for this, where construction from floats is supposed to be necessary too.

Thus, I updated the merge request s.t. it is quite conservative, with quite minimal modifications.
Most importantly, I added non-const partial_denominators getter, but as a protected member. This at least allows the user to use it as a base class and handle non-const operations at his/her own risk.

There is still one issue though. Since both const and non-const partial_denominators getters have the same name, the non-const getter is preferred by the compiler in any context where the instance of the class is non-const - which is quite common (one does not always use the instances as const variables). This results in calling the protected getter - compiler error. The user has to always use only const variables or cast the variable to be const (like with std::as_const), but I suppose this is incovenient. (I also think this will result in failing the CI tests.)
Thus, I would suggest to rename the non-const getter. But I am not familiar with Boost coding styles, so I do not know which name to choose...

I also added int_type type alias, consistently to boost::rational.
And I added defaulted default, copy and move constructors (I suppose that requiring std=c++11 is OK?)

[NAThompson]

I only found sources where either "Mathworld's definition" is used, or the coefficients b_i have no specific name.
So in the end, I suppose that the current form can be kept, I just added a note with clarification that it also includes the integer part.

IIRC, I was reading Khinchin's book when I wrote this class. I'll take a look at it and see whether I messed up and used a different definition. In either case, the documentation can be made more clear.

And I added defaulted default, copy and move constructors (I suppose that requiring std=c++11 is OK?)

Is this necessary? They should've been created by the compiler-or better explicitly deleted.

Most importantly, I added non-const partial_denominators getter, but as a protected member. This at least allows the user to use it as a base class and handle non-const operations at his/her own risk.

I confess I do not see a good reason to allow non-const modifications.

[Tomaqa]

I confess I do not see a good reason to allow non-const modifications.

Consider the following example, where I want to compute best rational approximation within an interval:

using boost::math::tools::simple_continued_fraction;

const RealT lhs = ...;
const RealT rhs = ...;

const auto cont_frac1 = simple_continued_fraction(lhs);
const auto cont_frac2 = simple_continued_fraction(rhs);
auto& coefs1 = cont_frac1.partial_denominators();
auto& coefs2 = cont_frac2.partial_denominators();

const size_t max_size = min(coefs1.size(), coefs2.size());
simple_continued_fraction cont_frac;
auto& coefs = cont_frac.partial_denominators();
coefs.reserve(max_size);
for (size_t i = 0; i < max_size; ++i) {
    const auto c1 = coefs1[i];
    const auto c2 = coefs2[i];
    if (c1 == c2) {
        coefs.push_back(c1);
        continue;
    }

    coefs.push_back(min(c1, c2) + 1);
    break;
}

return cont_frac;

It is not possible to construct such simple_continued_fraction with only const partial_denominators.

[mborland]

Suggested change

	simple_continued_fraction(std::vector<Z> data) : b_{std::move(data)} {
	simple_continued_fraction(std::vector<Z>&& data) : b_{data} {

As written this makes a copy of the data vector and then moves it into b_. I expect you want to take an r-value reference instead.

[Tomaqa]

simple_continued_fraction(std::vector<Z>) can be used both with std::vector<Z>&& and const std::vector<Z>& arguments, whereas simple_continued_fraction(std::vector<Z>&&) cannot be used with const std::vector<Z>&. In the case of std::vector<Z>&& argument, there should no significant difference (std::vector<Z> can require one more call to move constructor, which is negligible).

More importantly, I'm pretty sure that your suggestion simple_continued_fraction(std::vector<Z>&& data) : b_{data} will call copy constructor on data, because you pass it as an lvalue, not as an rvalue.

[mborland]

Does this work with other rational types (e.g. mpq_t)?

[Tomaqa]

I assume that mpq_t cannot be used since it is C-style struct which requires to call mpq_init etc.
The function can be used with any type T that supports typename T::int_type, T(typename T::int_type) (just integer part) and T(typename T::int_type, typename T::int_type) (numerator and denominator).
I assume C++20 is not supported so I do not use concepts or such.

[mborland]

Is this being superseded by #975?

[Tomaqa]

Is this being superseded by #975?

Totally, I did not know that it will connect back with this PR, so I assume #975 is redundant.