bugGNU Octave - Bugs: bug #55968, [octave forge] (symbolic)...

 
 

bug #55968: [octave forge] (symbolic) Incorrect simplify to sinc function

Submitter:  Z. M. <zmarantz>
Submitted:  Wed 20 Mar 2019 08:59:00 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Z.M. Open/Closed:  * Closed
Release:  * 4.4.0 Operating System:  * Mac OS
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 04 Apr 2019 06:27:19 PM UTC, comment #18: 

Thank you Z.M. and Mike.  I would not have done this fix without a bit of pushing as I assumed it would be too risky.  So thank you for helping make this software better.


Colin Macdonald <cbm>
Thu 04 Apr 2019 05:50:12 PM UTC, comment #17: 

This is now fixed in git, we were able to rewrite the args to the sinc function when it prints, so with the original example you now get


>> X = rewrite(X,'sinc')
X = (sym)

        ⎛ ω  ⎞
  3⋅sinc⎜────⎟
        ⎝10⋅π⎠
  ────────────
       10


Closing as fixed.

Mike Miller <mtmiller>
Group Member
Wed 03 Apr 2019 05:15:58 PM UTC, comment #16: 

We are playing with monkey-patching SymPy to improve all this at https://github.com/cbm755/octsympy/pull/954

Feel free to join the fun!

Colin Macdonald <cbm>
Wed 03 Apr 2019 04:53:10 PM UTC, comment #15: 

The inconsistency is still very disturbing.

Z. M. <zmarantz>
Tue 26 Mar 2019 10:03:16 PM UTC, comment #14: 


> And though you can symbolically utilize sinc in MATLAB as shown above, you can't in Octave:


This is fixed in git now:


>> syms q
>> sinc (q)
ans = (sym) sinc(π⋅q)


Here, again, you can see that calling 'sinc' on a symbolic variable uses the normalized sinc to be consistent with numeric Octave and Matlab, but SymPy only knows how to print it with respect to its own unnormalized 'sinc' function.

Mike Miller <mtmiller>
Group Member
Tue 26 Mar 2019 09:36:15 PM UTC, comment #13: 

@mtmiller: Evaluating numerically is great, but "You will have to ignore the way that it looks" seems far from something that should be acceptable here.

Shouldn't the symbolic part be consistent, too? 

Maybe is should be disabled altogether, since it's not possible in MATLAB:

>> syms t x
>> x=sinc(t)

 
x =
 
sin(pi*t)/(t*pi)
 

>> rewrite(x,'sinc')

 
ans =
 
sin(pi*t)/(t*pi) % No change

And though you can symbolically utilize sinc in MATLAB as shown above, you can't in Octave:

>> syms q

Symbolic pkg v2.7.1: Python communication link active, SymPy v1.3.

>> sinc(q)

error: subscript indices must be integers or boolean
error: called from
    subsindex at line 64 column 5
    subsref at line 60 column 22
    sinc at line 45 column 7


Z. M. <zmarantz>
Tue 26 Mar 2019 07:22:42 PM UTC, comment #12: 

I mentioned this earlier but here's a concrete example of function arguments swapping order in SymPy versus Octave.  See `help @sym/gammainc`:


          syms x a
          gammainc(x, a)
            ⇒ (sym) γ(a, x)
          gammainc(x,a, 'upper')
            ⇒ (sym) Γ(a, x)


Again, maybe its possible to monkey-patch SymPy... but not sure its wise.  But at least I should add notes to the docs of these functions!  Filed https://github.com/cbm755/octsympy/issues/952

Colin Macdonald <cbm>
Tue 26 Mar 2019 06:35:58 PM UTC, comment #11: 

The only ways I can think to deal with this in a satisfactory way are:

1.  find/replace on the srepr string and call it something else that is exactly 4 chars long: "sncu" or "siNc', 'SINC'

2.  long monkey patch to sympy so that it uses the normalized sinc.  This could be tricky, bit of grepping says 97 occurrences in the source

3.  very short monkey-patch to sympy's printing system to replace the name with `sinc_unnorm` or similar.  This might be the least unpleasant.  But OTOH I dislike diverging from SymPy.

4.  Leave it alone, confuse end-users who don't read the manual (and probably those who do read the manual ;-)

Colin Macdonald <cbm>
Tue 26 Mar 2019 03:52:12 AM UTC, comment #10: 

@Z.M. It depends what you mean by "work". In your original example, the result of rewrite will be a symbolic variable. If you 'disp' that variable, it will print itself as if it were "sinc(w/10)"


      ⎛ω ⎞
  sinc⎜──⎟
      ⎝10⎠


You will have to ignore the way that it looks, because Python is generating that appearance.

But if you evaluate that function, or turn it into an inline Octave function, it will evaluate to the right thing in terms of Octave's normalized sinc function.


>> double (subs (fn, 10))
ans =  0.84147
>> f = matlabFunction (fn)
f =

@(omega) sinc (omega / (10 * pi))

>> f(10)
ans =  0.84147


Mike Miller <mtmiller>
Group Member
Tue 26 Mar 2019 02:57:39 AM UTC, comment #9: 

@cbm: The explanation looks fine in the comments and it does clarify the issue, but will the original code that was submitted in this thread work as anticipated as a result?

Z. M. <zmarantz>
Fri 22 Mar 2019 09:42:48 PM UTC, comment #8: 

Can you both review this pull request?

https://github.com/cbm755/octsympy/pull/946

I think it addresses the original issue here, but I'd like some proofreading of my explanations!

Colin Macdonald <cbm>
Fri 22 Mar 2019 08:59:59 PM UTC, comment #7: 

We are discussing two different issues here.

@mtmiller: I've filed https://github.com/cbm755/octsympy/issues/945 for your's.

@zmarantz: we do use that solution: `octave_code` (the relevant sympy function) gives equivalent octave code in terms of the normalized sinc function.  I will see if I can write `help sinc` in way that tries to clarify this.  I will ask you both to review it...

Colin Macdonald <cbm>
Fri 22 Mar 2019 05:01:51 PM UTC, comment #6: 

Is there any way to utilize this solution:

https://github.com/sympy/sympy/issues/13642 ?

Z. M. <zmarantz>
Thu 21 Mar 2019 05:17:51 PM UTC, comment #5: 

Ok, by way of example, in Matlab:


>> rewrite(tan(x), 'sin')
ans =
    - sin(x) / (2 * sin(x/2)^2 - 1)

>> rewrite(tan(x), 'sinc')
ans =
    sin(x) / cos(x)

>> rewrite(tan(x), 'sinco')
ans =
    sin(x) / cos(x)

>> rewrite(tan(x), 'sincos')
ans =
    sin(x) / cos(x)

>> rewrite(tan(x), 'asdfasdf')
Error using rewrite (line 164)
Expected input to match one of these values:



In Octave


>> rewrite(tan(x), 'sin')
ans = (sym) 2*sin(x)**2/sin(2*x)

>> rewrite(tan(x), 'sinc')
ans = (sym) tan(x)

>> rewrite(tan(x), 'sinco')
ans = (sym) tan(x)

>> rewrite(tan(x), 'sincos')
ans = (sym) sin(x)/cos(x)

>> rewrite(tan(x), 'asdfasdf')
ans = (sym) tan(x)


SymPy does support "sincos". I am suggesting that if we want to be compatible with SMT in this case, the argument to rewrite "sinc" should be treated the same as "sincos", and whether sinc is defined normalized or unnormalized is moot.

Mike Miller <mtmiller>
Group Member
Thu 21 Mar 2019 10:50:45 AM UTC, comment #4: 

I'm not sure I understand...  I think rewrite is functioning correctly, given the situation:

  * Octave (and Matlab) have a sinc(double) function.  It is normalized.

  * SymPy has a sinc function.  It is unnormalized.

Symbolic could/should add @sym/sinc.m, it will necessarily be:


function y = sinc(x)
  y = elementwise_op ('sinc', pi*x);
end


And its docs will have a big warning about normalized/unnormalized: that's the only thing I can think to do to resolve the original issue.  A similar issue comes up when a function in SymPy has its arguments in a different order than we do or is spelled differently.

Or are you noting a different compatibility bug?  Namely that SMT supports "sincos" and "sinhcosh" but we don't.  That's valid.  But what does it have to do with "sinc"?

Colin Macdonald <cbm>
Thu 21 Mar 2019 01:51:17 AM UTC, comment #3: 

Minor correction, SymPy does not accept 'sinco', it simply doesn't rewrite anything if the argument isn't recognized. So "rewrite(f, 'sinco')" is the same as "rewrite(f, 'zzzzzzzz')".

This bug could still be fixed by intercepting and looking for the specific names "sinc" and "sinco", and translating them into "sincos" for Matlab compatibility.

Mike Miller <mtmiller>
Group Member
Thu 21 Mar 2019 01:17:48 AM UTC, comment #2: 

Here's an idea of what we could do. In Matlab, they are a little more restrictive over what can be passed in the second argument to rewrite. If I pass an expression to rewrite with the target 'sinc', it's actually interpreted by Matlab as a synonym for 'sincos'. So is the option 'sinco'.

SymPy seems to handle 'sinco' as an alias for 'sincos' already. We could simply intercept and if the argument is exactly 'sinc', translate it into 'sincos' before handing it off to SymPy. What do you think?

Mike Miller <mtmiller>
Group Member
Thu 21 Mar 2019 12:07:19 AM UTC, comment #1: 

SymPy's sinc is "unnormalized".

I think if you do `function_handle(X)` it should normalize again (because it is rewriting it into Octave code).

There isn't much we can do about this...  I haven't included a sinc.m yet in Symbolic, but on my local installation:

>> sinc(4.2)
ans =   0.044547

>> f = sinc(x)     % note pi being introduced
f = (sym) sinc(π⋅x)
>> subs(f, x, sym(42)/10)
ans = (sym)

      ⎛21⋅π⎞
  sinc⎜────⎟
      ⎝ 5  ⎠

>> double(ans)    % and this should match
ans =   0.044547


So the output looks confusing but I don't think there is anything except an unfixable cosmetic bug here.

It does highlight how the various glue between Octave (Symbolic package) and SymPy ("octave_code") needs to be considered carefully on a case-by-case basis, not just blindly machine generated.

Colin Macdonald <cbm>
Wed 20 Mar 2019 08:59:00 PM UTC, original submission:  

Hi Maintainer,

I am running Octave 4.4.0 on MAC OSX.

I ran the following code:

tau=0.2; A = 1.5;
syms t x(t) omega
x(t) = A*(heaviside(t+tau/2) - heaviside(t-tau/2));
X = fourier(x(t),t,omega)
X = simplify(X)
X = rewrite(X,'sinc')

The result is shown in the attached file.

As you can see, I get sinc(w / 10).

Unfortunately, the correct answer in terms of sinc should be: sinc(w/(10pi)) especially if sinc is defined as: sinc(x) = sin(pi*x)/(pi*x).

Any assistance that you could provide would be greatly appreciated.

Thank you,
Z.M.

Z. M. <zmarantz>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by cbm (Posted a comment)
  • -email is unavailable- added by zmarantz (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-04-04 mtmiller StatusNone Fixed
        Open/ClosedOpen Closed
    2019-03-21 mtmiller SummaryIncorrect simplify to sinc function [octave forge] (symbolic) Incorrect simplify to sinc function
    2019-03-20 zmarantz Attached File- Added Screen Shot 2019-03-20 at 4.55.38 PM.png, #46595

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code