[PATCH 1/8] inifcns_nstdsums.cpp: S_num takes cl_N as an argument instead of numeric.

[Alexei Sheplyakov-2]

Implicit conversion from cl_N to numeric considered harmful.

Using GiNaC::numeric for numerical computations incurs significant
overhead, so one might want to do these computations using proper CLN
types. Unfortunately, it's not easy due to automatic conversion from
cln::cl_N to GiNaC::numeric. Here is a simple example:

cl_N x, y;
// initialize them
return sin(x) +  y*exp(y);

The compiler complains about ambigously overloaded of functions, i.e.
cl_N cln::sin(const cl_N&) versus numeric GiNaC::sin(const numeric&).

Hence, I propose to disable *implicit* conversion from cl_N to numeric
(this can be done by marking the numeric ctor as `explicit').

However, this change happens to be a bit nontrivial, because that evil
conversion is used in GiNaC itself. So, I decided to rewrite those fragments
of code.

---
 ginac/inifcns_nstdsums.cpp |   50 ++++++++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/ginac/inifcns_nstdsums.cpp b/ginac/inifcns_nstdsums.cpp
index cd3511a..ea42a6e 100644
--- a/ginac/inifcns_nstdsums.cpp
+++ b/ginac/inifcns_nstdsums.cpp
@@ -320,7 +320,7 @@ cln::cl_N Lin_do_sum_Xn(int n, const cln::cl_N& x)
 
 
 // forward declaration needed by function Li_projection and C below
-numeric S_num(int n, int p, const numeric& x);
+const cln::cl_N S_num(int n, int p, const cln::cl_N& x);
 
 
 // helper function for classical polylog Li
@@ -371,7 +371,7 @@ cln::cl_N Li_projection(int n, const cln::cl_N& x, const cln::float_format_t& pr
  } else {
  cln::cl_N result = -cln::expt(cln::log(x), n-1) * cln::log(1-x) / cln::factorial(n-1);
  for (int j=0; j<n-1; j++) {
- result = result + (S_num(n-j-1, 1, 1).to_cl_N() - S_num(1, n-j-1, 1-x).to_cl_N())
+ result = result + (S_num(n-j-1, 1, 1) - S_num(1, n-j-1, 1-x))
                   * cln::expt(cln::log(x), j) / cln::factorial(j);
  }
  return result;
@@ -402,7 +402,7 @@ numeric Lin_numeric(int n, const numeric& x)
  cln::cl_N x_ = ex_to<numeric>(x).to_cl_N();
  cln::cl_N result = -cln::expt(cln::log(x_), n-1) * cln::log(1-x_) / cln::factorial(n-1);
  for (int j=0; j<n-1; j++) {
- result = result + (S_num(n-j-1, 1, 1).to_cl_N() - S_num(1, n-j-1, 1-x_).to_cl_N())
+ result = result + (S_num(n-j-1, 1, 1) - S_num(1, n-j-1, 1-x_))
  * cln::expt(cln::log(x_), j) / cln::factorial(j);
  }
  return result;
@@ -1715,10 +1715,10 @@ cln::cl_N C(int n, int p)
  if (k == 0) {
  if (n & 1) {
  if (j & 1) {
- result = result - 2 * cln::expt(cln::pi(),2*j) * S_num(n-2*j,p,1).to_cl_N() / cln::factorial(2*j);
+ result = result - 2 * cln::expt(cln::pi(),2*j) * S_num(n-2*j,p,1) / cln::factorial(2*j);
  }
  else {
- result = result + 2 * cln::expt(cln::pi(),2*j) * S_num(n-2*j,p,1).to_cl_N() / cln::factorial(2*j);
+ result = result + 2 * cln::expt(cln::pi(),2*j) * S_num(n-2*j,p,1) / cln::factorial(2*j);
  }
  }
  }
@@ -1726,23 +1726,23 @@ cln::cl_N C(int n, int p)
  if (k & 1) {
  if (j & 1) {
  result = result + cln::factorial(n+k-1)
-                  * cln::expt(cln::pi(),2*j) * S_num(n+k-2*j,p-k,1).to_cl_N()
+                  * cln::expt(cln::pi(),2*j) * S_num(n+k-2*j,p-k,1)
                   / (cln::factorial(k) * cln::factorial(n-1) * cln::factorial(2*j));
  }
  else {
  result = result - cln::factorial(n+k-1)
-                  * cln::expt(cln::pi(),2*j) * S_num(n+k-2*j,p-k,1).to_cl_N()
+                  * cln::expt(cln::pi(),2*j) * S_num(n+k-2*j,p-k,1)
                   / (cln::factorial(k) * cln::factorial(n-1) * cln::factorial(2*j));
  }
  }
  else {
  if (j & 1) {
- result = result - cln::factorial(n+k-1) * cln::expt(cln::pi(),2*j) * S_num(n+k-2*j,p-k,1).to_cl_N()
+ result = result - cln::factorial(n+k-1) * cln::expt(cln::pi(),2*j) * S_num(n+k-2*j,p-k,1)
                   / (cln::factorial(k) * cln::factorial(n-1) * cln::factorial(2*j));
  }
  else {
  result = result + cln::factorial(n+k-1)
-                  * cln::expt(cln::pi(),2*j) * S_num(n+k-2*j,p-k,1).to_cl_N()
+                  * cln::expt(cln::pi(),2*j) * S_num(n+k-2*j,p-k,1)
                   / (cln::factorial(k) * cln::factorial(n-1) * cln::factorial(2*j));
  }
  }
@@ -1855,7 +1855,7 @@ cln::cl_N S_projection(int n, int p, const cln::cl_N& x, const cln::float_format
  res2 = res2 + cln::expt(cln::cl_I(-1),r) * cln::expt(cln::log(1-x),r)
               * S_do_sum(p-r,n-s,1-x,prec) / cln::factorial(r);
  }
- result = result + cln::expt(cln::log(x),s) * (S_num(n-s,p,1).to_cl_N() - res2) / cln::factorial(s);
+ result = result + cln::expt(cln::log(x),s) * (S_num(n-s,p,1) - res2) / cln::factorial(s);
  }
 
  return result;
@@ -1866,7 +1866,7 @@ cln::cl_N S_projection(int n, int p, const cln::cl_N& x, const cln::float_format
 
 
 // helper function for S(n,p,x)
-numeric S_num(int n, int p, const numeric& x)
+const cln::cl_N S_num(int n, int p, const cln::cl_N& x)
 {
  if (x == 1) {
  if (n == 1) {
@@ -1902,11 +1902,11 @@ numeric S_num(int n, int p, const numeric& x)
  // what is the desired float format?
  // first guess: default format
  cln::float_format_t prec = cln::default_float_format;
- const cln::cl_N value = x.to_cl_N();
+ const cln::cl_N value = x;
  // second guess: the argument's format
- if (!x.real().is_rational())
+ if (!instanceof(realpart(value), cln::cl_RA_ring))
  prec = cln::float_format(cln::the<cln::cl_F>(cln::realpart(value)));
- else if (!x.imag().is_rational())
+ else if (!instanceof(imagpart(value), cln::cl_RA_ring))
  prec = cln::float_format(cln::the<cln::cl_F>(cln::imagpart(value)));
 
  // [Kol] (5.3)
@@ -1919,9 +1919,9 @@ numeric S_num(int n, int p, const numeric& x)
  cln::cl_N res2;
  for (int r=0; r<p; r++) {
  res2 = res2 + cln::expt(cln::cl_I(-1),r) * cln::expt(cln::log(1-value),r)
-              * S_num(p-r,n-s,1-value).to_cl_N() / cln::factorial(r);
+              * S_num(p-r,n-s,1-value) / cln::factorial(r);
  }
- result = result + cln::expt(cln::log(value),s) * (S_num(n-s,p,1).to_cl_N() - res2) / cln::factorial(s);
+ result = result + cln::expt(cln::log(value),s) * (S_num(n-s,p,1) - res2) / cln::factorial(s);
  }
 
  return result;
@@ -1936,7 +1936,7 @@ numeric S_num(int n, int p, const numeric& x)
  for (int r=0; r<=s; r++) {
  result = result + cln::expt(cln::cl_I(-1),s) * cln::expt(cln::log(-value),r) * cln::factorial(n+s-r-1)
                   / cln::factorial(r) / cln::factorial(s-r) / cln::factorial(n-1)
-                  * S_num(n+s-r,p-s,cln::recip(value)).to_cl_N();
+                  * S_num(n+s-r,p-s,cln::recip(value));
  }
  }
  result = result * cln::expt(cln::cl_I(-1),n);
@@ -1972,12 +1972,18 @@ numeric S_num(int n, int p, const numeric& x)
 static ex S_evalf(const ex& n, const ex& p, const ex& x)
 {
  if (n.info(info_flags::posint) && p.info(info_flags::posint)) {
+ const int n_ = ex_to<numeric>(n).to_int();
+ const int p_ = ex_to<numeric>(p).to_int();
  if (is_a<numeric>(x)) {
- return S_num(ex_to<numeric>(n).to_int(), ex_to<numeric>(p).to_int(), ex_to<numeric>(x));
+ const cln::cl_N x_ = ex_to<numeric>(x).to_cl_N();
+ const cln::cl_N result = S_num(n_, p_, x_);
+ return numeric(result);
  } else {
  ex x_val = x.evalf();
  if (is_a<numeric>(x_val)) {
- return S_num(ex_to<numeric>(n).to_int(), ex_to<numeric>(p).to_int(), ex_to<numeric>(x_val));
+ const cln::cl_N x_val_ = ex_to<numeric>(x_val).to_cl_N();
+ const cln::cl_N result = S_num(n_, p_, x_val_);
+ return numeric(result);
  }
  }
  }
@@ -2002,7 +2008,11 @@ static ex S_eval(const ex& n, const ex& p, const ex& x)
  return Li(n+1, x);
  }
  if (x.info(info_flags::numeric) && (!x.info(info_flags::crational))) {
- return S_num(ex_to<numeric>(n).to_int(), ex_to<numeric>(p).to_int(), ex_to<numeric>(x));
+ int n_ = ex_to<numeric>(n).to_int();
+ int p_ = ex_to<numeric>(p).to_int();
+ const cln::cl_N x_ = ex_to<numeric>(x).to_cl_N();
+ const cln::cl_N result = S_num(n_, p_, x_);
+ return numeric(result);
  }
  }
  if (n.is_zero()) {
--
1.5.4.2


--
All science is either physics or stamp collecting.



_______________________________________________
GiNaC-devel mailing list
GiNaC-devel@...
https://www.cebix.net/mailman/listinfo/ginac-devel

[Richard B. Kreckel]

Dear Alexei,

Alexei Sheplyakov wrote:
> Implicit conversion from cl_N to numeric considered harmful.

I agree: It's quite a greedy mechanism that does have its pitfalls.

> Using GiNaC::numeric for numerical computations incurs significant
> overhead, so one might want to do these computations using proper CLN
> types. Unfortunately, it's not easy due to automatic conversion from
> cln::cl_N to GiNaC::numeric. Here is a simple example:
>
> cl_N x, y;
> // initialize them
> return sin(x) +  y*exp(y);
>
> The compiler complains about ambigously overloaded of functions, i.e.
> cl_N cln::sin(const cl_N&) versus numeric GiNaC::sin(const numeric&).

Does it? Can you provide a complete example? This program compiles fine:

#include <cln/cln.h>
#include <ginac/ginac.h>
using namespace GiNaC;
using namespace cln;
int main()
{
     cl_N x, y;
     // initialize them
     numeric result = sin(x) +  y*exp(y);
}

> Hence, I propose to disable *implicit* conversion from cl_N to numeric
> (this can be done by marking the numeric ctor as `explicit').

Or, alternatively, by making it private! (But I'm not sure if this does
eliminate all ambiguities.)

> However, this change happens to be a bit nontrivial, because that evil
> conversion is used in GiNaC itself. So, I decided to rewrite those fragments
> of code.

Is there a reason you haven't committed it somewhere? I'm a beginner
with git, but it would appear to me that this could be handled easily
with a branch, right?

Best wishes
   -richy.

PS: I've pushed your other changes.
--
Richard B. Kreckel
<http://www.ginac.de/~kreckel/>
_______________________________________________
GiNaC-devel mailing list
GiNaC-devel@...
https://www.cebix.net/mailman/listinfo/ginac-devel

[Richard B. Kreckel]

Richard B. Kreckel wrote:
>> Hence, I propose to disable *implicit* conversion from cl_N to numeric
>> (this can be done by marking the numeric ctor as `explicit').
>
> Or, alternatively, by making it private!

Oh, scratch this. Nobody wants to break third-party code that uses both
GiNaC and CLN.

   -richy.
--
Richard B. Kreckel
<http://www.ginac.de/~kreckel/>
_______________________________________________
GiNaC-devel mailing list
GiNaC-devel@...
https://www.cebix.net/mailman/listinfo/ginac-devel

[Alexei Sheplyakov-2]

Dear Richard,

On Sat, Mar 22, 2008 at 12:28:07AM +0100, Richard B. Kreckel wrote:

> > Using GiNaC::numeric for numerical computations incurs significant
> > overhead, so one might want to do these computations using proper CLN
> > types. Unfortunately, it's not easy due to automatic conversion from
> > cln::cl_N to GiNaC::numeric. Here is a simple example:
> >
> > cl_N x, y;
> > // initialize them
> > return sin(x) +  y*exp(y);
> >
> > The compiler complains about ambigously overloaded of functions, i.e.
> > cl_N cln::sin(const cl_N&) versus numeric GiNaC::sin(const numeric&).
>
> Does it? Can you provide a complete example?

$ cat numconv2.cpp

#include <cln/cln.h>
#include <ginac/ginac.h>
#include <iostream>
using namespace GiNaC;

int main(int argc, char** argv)
{
        symbol a("a");
        cln::cl_N x = cln::cl_float(2), y = cln::pi()/6;
        ex e = a*(1 + log(x + y)*y + x*cos(y));
        std::cout << e << std::endl;
        return 0;
}

$ g++ -Wall -O0 -g numconv2.cpp -lginac -lcln

numconv2.cpp: In function ‘int main(int, char**)’:
numconv2.cpp:10: error: no match for ‘operator*’ in ‘a * cln::operator+(const cln::cl_N&, const cln::cl_N&)(((const cln::cl_N&)(& cln::operator*(const cln::cl_
N&, const cln::cl_N&)(((const cln::cl_N&)(& cln::cos(const cln::cl_N&)()))))))’
/usr/include/ginac/operators.h:37: note: candidates are: const GiNaC::ex GiNaC::operator*(const GiNaC::ex&, const GiNaC::ex&)
/usr/include/ginac/operators.h:43: note:                 const GiNaC::numeric GiNaC::operator*(const GiNaC::numeric&, const GiNaC::numeric&)
/usr/include/cln/univpoly_modint.h:188: note:                 const cln::cl_UP_MI cln::operator*(const cln::cl_UP_MI&, const cln::cl_MI&)
[skipped the rest]

But my analysis seems to be wrong. Anyway, I'd like to get rid of implicit
conversion, since it's very confusing.

> Is there a reason you haven't committed it somewhere?

Yes. It's a good idea to discuss a patch before commiting it to a public
branch.

> I'm a beginner with git, but it would appear to me that this could
> be handled easily with a branch, right?

Actually, it is handled with a branch. But this brunch is not public
(so I can rebase it, delete wrong commits, etc., without confusing people
who cloned my repository).

Best regards,
        Alexei

--
All science is either physics or stamp collecting.



_______________________________________________
GiNaC-devel mailing list
GiNaC-devel@...
https://www.cebix.net/mailman/listinfo/ginac-devel

[Richard B. Kreckel]

Dear Alexei,

Alexei Sheplyakov wrote:

>>> Using GiNaC::numeric for numerical computations incurs significant
>>> overhead, so one might want to do these computations using proper CLN
>>> types. Unfortunately, it's not easy due to automatic conversion from
>>> cln::cl_N to GiNaC::numeric. Here is a simple example:
>>>
>>> cl_N x, y;
>>> // initialize them
>>> return sin(x) +  y*exp(y);
>>>
>>> The compiler complains about ambigously overloaded of functions, i.e.
>>> cl_N cln::sin(const cl_N&) versus numeric GiNaC::sin(const numeric&).
>> Does it? Can you provide a complete example?
>
> $ cat numconv2.cpp
>
> #include <cln/cln.h>
> #include <ginac/ginac.h>
> #include <iostream>
> using namespace GiNaC;
>
> int main(int argc, char** argv)
> {
> symbol a("a");
> cln::cl_N x = cln::cl_float(2), y = cln::pi()/6;
> ex e = a*(1 + log(x + y)*y + x*cos(y));
> std::cout << e << std::endl;
> return 0;
> }
>
> $ g++ -Wall -O0 -g numconv2.cpp -lginac -lcln
>
> numconv2.cpp: In function ‘int main(int, char**)’:
> numconv2.cpp:10: error: no match for ‘operator*’ in ‘a * cln::operator+(const cln::cl_N&, const cln::cl_N&)(((const cln::cl_N&)(& cln::operator*(const cln::cl_
> N&, const cln::cl_N&)(((const cln::cl_N&)(& cln::cos(const cln::cl_N&)()))))))’
> /usr/include/ginac/operators.h:37: note: candidates are: const GiNaC::ex GiNaC::operator*(const GiNaC::ex&, const GiNaC::ex&)
> /usr/include/ginac/operators.h:43: note:                 const GiNaC::numeric GiNaC::operator*(const GiNaC::numeric&, const GiNaC::numeric&)
> /usr/include/cln/univpoly_modint.h:188: note:                 const cln::cl_UP_MI cln::operator*(const cln::cl_UP_MI&, const cln::cl_MI&)
> [skipped the rest]
>
> But my analysis seems to be wrong.

Well, the example is calling for an operator*(symbol, cl_N) and the
compiler's only matches are operator*(ex, ex) and operator*(cl_<type1>,
cl_<type2>). Class numeric isn't involved and making that ctor explicit
won't help.

> Anyway, I'd like to get rid of implicit
> conversion, since it's very confusing.

That conversion seems to be rarely used except inside GiNaC proper. At
least Orsa, nestedsums, and xloops do not use it. I don't think anyone
would miss its removal.

   -richy.
--
Richard B. Kreckel
<http://www.ginac.de/~kreckel/>
_______________________________________________
GiNaC-devel mailing list
GiNaC-devel@...
https://www.cebix.net/mailman/listinfo/ginac-devel

[Richard B. Kreckel]

Hi,

Back in March, Alexei posted a series of eight patches which made the
conversion from cln::cl_N to GiNaC::numeric explicit. There were no
objections to such a change.

Alexei Sheplyakov wrote:
>> I'm a beginner with git, but it would appear to me that this could
>> be handled easily with a branch, right?
>
> Actually, it is handled with a branch. But this brunch is not public
> (so I can rebase it, delete wrong commits, etc., without confusing people
> who cloned my repository).

Can we arrange to merge this patch into the master branch?

Cheers
   -richy.
--
Richard B. Kreckel
<http://www.ginac.de/~kreckel/>
_______________________________________________
GiNaC-devel mailing list
GiNaC-devel@...
https://www.cebix.net/mailman/listinfo/ginac-devel

[Jens Vollinga]

Hi,

Richard B. Kreckel schrieb:
> Back in March, Alexei posted a series of eight patches which made the
> conversion from cln::cl_N to GiNaC::numeric explicit. There were no
> objections to such a change.
 >
> Can we arrange to merge this patch into the master branch?

I can/will do that this evening.

Regards,
Jens

_______________________________________________
GiNaC-devel mailing list
GiNaC-devel@...
https://www.cebix.net/mailman/listinfo/ginac-devel