Opened 3 years ago

Closed 3 years ago

#11352 closed defect (fixed)

is_polynomial returns wrong results

Reported by: hivert Owned by: burcin
Priority: major Milestone: sage-4.7.1
Component: symbolics Keywords: is_polynomial
Cc: kcrisman, burcin, hivert, rbk Merged in: sage-4.7.1.alpha4
Authors: Richard Kreckel, Jens Vollinga, Burcin Erocal Reviewers: Karl-Dieter Crisman
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: Commit:
Dependencies: #11415 Stopgaps:

Description (last modified by kcrisman)

sage: el = -1/2*(2*x^2 - sqrt(2*x - 1)*sqrt(2*x + 1) - 1)
sage: el.is_polynomial(x)
True

I definitely prefer to get False.

Apply trac_11352-is_polynomial.patch.

Attachments (1)

trac_11352-is_polynomial.patch (825 bytes) - added by burcin 3 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 follow-up: Changed 3 years ago by vbraun

We just return GiNaC's response:

  cdef Expression symbol0 = self.coerce_in(var)
  return self._gobj.is_polynomial(symbol0._gobj)

comment:2 in reply to: ↑ 1 Changed 3 years ago by hivert

Replying to vbraun:

We just return GiNaC's response:

  cdef Expression symbol0 = self.coerce_in(var)
  return self._gobj.is_polynomial(symbol0._gobj)

So this probably must be reported to GiNaC, isn't it ?

comment:3 Changed 3 years ago by hivert

  • Description modified (diff)
  • Owner changed from burcin to hivert

comment:4 Changed 3 years ago by kcrisman

  • Cc kcrisman burcin added

comment:5 follow-up: Changed 3 years ago by hivert

  • Report Upstream changed from N/A to Reported upstream. Little or no feedback.

I just installed a fresh GiNaC on my computer and I've been able to reproduce the bug:

#include <iostream>
#include <ginac/ginac.h>

using namespace std;
using namespace GiNaC;

int main()
{
  symbol x("x");
  ex poly;

  poly = sqrt(x*x+1)*sqrt(x+1);

  cout << poly << endl;
  cout << poly.is_polynomial(x) << endl;
  return 0;
}

gives

sqrt(1+x^2)*sqrt(1+x)
1

So I subscribed to ginac mailing list and reported the bug here. I'm waiting for an answer.

Florent

comment:6 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by kcrisman

So I subscribed to ginac mailing list and reported the bug here. I'm waiting for an answer.

Thanks for doing that, Florent. By the way, can you put a link to the thread on the Ginac list here?

comment:7 in reply to: ↑ 6 Changed 3 years ago by hivert

Thanks for doing that, Florent. By the way, can you put a link to the thread on the Ginac list here?

Sure ! Here it is:

http://www.ginac.de/pipermail/ginac-list/2011-May/001822.html

comment:8 Changed 3 years ago by hivert

  • Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. Developers acknowledge bug.
  • Status changed from new to needs_info
  • Work issues set to wait for upstream fix

I just got the following answer:

thanks for reporting the bug. I am about to fix it.

I'm not sure what will happen next: Should we backport the fix or update the whole ginac ?

comment:9 Changed 3 years ago by hivert

  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, in a later stable release.
  • Work issues wait for upstream fix deleted

The bug seems to be fixed upstream, thanks to Richard B. Kreckel:

Anyway, in this case you'll have to port these two patches to Pynac:

http://www.ginac.de/ginac.git?p=ginac.git;a=commitdiff;h=0c2f0f4c6d

http://www.ginac.de/ginac.git?p=ginac.git;a=commitdiff;h=293ff6f6fe

As I previously said, I'm not sure what to do next. Moreover, I'd rather let who knows about PyNaC and Sage symbolic internal handle the rests of this ticket. Please feel free to take ownership of it.

comment:10 follow-up: Changed 3 years ago by burcin

  • Owner changed from hivert to burcin

Thanks for following this through so far Florent. The next step is for me to make a new Pynac release with these patches. I will do this in the next few days, but it might be a while before this lands in a Sage release, since the next version of pynac will include fixes for #9880 and an upstream patch for #10964.

comment:11 in reply to: ↑ 10 Changed 3 years ago by hivert

  • Cc hivert added

Hi Burcin,

Replying to burcin:

Thanks for following this through so far Florent. The next step is for me to make a new Pynac release with these patches. I will do this in the next few days, but it might be a while before this lands in a Sage release, since the next version of pynac will include fixes for #9880 and an upstream patch for #10964.

I'm keeping an eye here. Count me as a volunteer if some help is needed for the review.

comment:12 Changed 3 years ago by rbk

  • Cc rbk added

Changed 3 years ago by burcin

comment:13 Changed 3 years ago by hivert

From the Ginac mailing list:

Hi everybody,

GiNaC 1.6.0 is out and available. You can find the changes at the end
of this email.

Note:
This release is not binary compatible to the previous one.
Also, we changed the name of the library filename from libginac-1.5 to
libginac. The ABI versioning is now done in recommended libtool
manner. The dynamic library will be created as libginac.so.2.0.0 this
time.
We also stopped to create extra branches in the repository for ABI
compatible versions. The latest releases will from now on always be in
master.

Changes:
[...]
* Fixed a bug in is_polynomial().
[...]

So we now "only" need to upgrade ginac.

There is no "fixed upstream and released" tag... I'm not sure if the current tag is correct. Florent

comment:14 Changed 3 years ago by burcin

I won't rant about the tags. :) Note that this is still needs_info.

Upstream is pynac in this case. I ported the patches in the GiNaC release to Pynac (this is easier than it sounds, you just ignore changes to files that don't exist), but didn't make a release yet. Here is the queue of patches which will be merged:

https://bitbucket.org/burcin/pynac-patches/src

comment:15 Changed 3 years ago by burcin

  • Authors set to Richard Kreckel, Jens Vollinga, Burcin Erocal
  • Dependencies set to #11415
  • Description modified (diff)
  • Milestone changed from sage-4.7.1 to sage-5.0
  • Status changed from needs_info to needs_review

New pynac package which includes the patch for this is at #11415.

attachment:trac_11352-is_polynomial.patch adds doctests.

comment:16 Changed 3 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

Positive review.

comment:17 Changed 3 years ago by kcrisman

  • Description modified (diff)

comment:18 Changed 3 years ago by kcrisman

  • Status changed from positive_review to needs_work

comment:19 Changed 3 years ago by kcrisman

  • Status changed from needs_work to needs_review

Oops, browser windows closed too fast - I was on #10964 still :)

comment:20 Changed 3 years ago by kcrisman

  • Description modified (diff)

Wiki markup here is different than on Sage wiki :(

comment:21 Changed 3 years ago by kcrisman

  • Status changed from needs_review to positive_review

Okay, now positive review! Good catch by rbk, incidentally, with the sqrt(2)*x issue.

comment:22 Changed 3 years ago by jdemeyer

  • Milestone changed from sage-5.0 to sage-4.7.1

comment:23 Changed 3 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.