Opened 5 years ago
Closed 5 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.
Attachments (1)
Change History (24)
comment:1 follow-up: ↓ 2 Changed 5 years ago by vbraun
comment:2 in reply to: ↑ 1 Changed 5 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 5 years ago by hivert
- Description modified (diff)
- Owner changed from burcin to hivert
comment:4 Changed 5 years ago by kcrisman
- Cc kcrisman burcin added
comment:5 follow-up: ↓ 6 Changed 5 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: ↓ 7 Changed 5 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 5 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 5 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 5 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: ↓ 11 Changed 5 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 5 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 5 years ago by rbk
- Cc rbk added
Changed 5 years ago by burcin
comment:13 Changed 5 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 5 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:
comment:15 Changed 5 years ago by burcin
- 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 5 years ago by kcrisman
- Reviewers set to Karl-Dieter Crisman
- Status changed from needs_review to positive_review
Positive review.
comment:17 Changed 5 years ago by kcrisman
- Description modified (diff)
comment:18 Changed 5 years ago by kcrisman
- Status changed from positive_review to needs_work
comment:19 Changed 5 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 5 years ago by kcrisman
- Description modified (diff)
Wiki markup here is different than on Sage wiki :(
comment:21 Changed 5 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 5 years ago by jdemeyer
- Milestone changed from sage-5.0 to sage-4.7.1
comment:23 Changed 5 years ago by jdemeyer
- Merged in set to sage-4.7.1.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
We just return GiNaC's response: