bugGNU Octave - Bugs: bug #32533, Operator precedence for transpose...

 
 

bug #32533: Operator precedence for transpose incorrect

Submitter:  Patrick Häcker <magicmuscleman>
Submitted:  Fri 18 Feb 2011 01:58:07 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  3 - Low Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 3.4.0 Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 21 Apr 2011 09:53:13 PM UTC, comment #21: 

OK, I updated the log message and committed the change to the default branch.  I'm marking this report as fixed and closing it.  If there are other problems, please open a new bug report.

John W. Eaton <jwe>
Group administrator
Thu 21 Apr 2011 09:40:38 PM UTC, comment #20: 

Yay!  I think we're finally done with this patch.

The only thing I would change would be to move the (bug #xxxxx) reference into the first summary line of the commit message.

'hg log' only shows the first line and it is convenient to use 'hg log | grep "bug #"' to find commits for bug reports.

Rik <rik5>
Group administrator
Thu 21 Apr 2011 08:42:20 PM UTC, comment #19: 

Before committing, I need to update the commit message to mention the lexer changes.  If this changeset is OK, let me know and I'll commit it.

John W. Eaton <jwe>
Group administrator
Thu 21 Apr 2011 08:40:29 PM UTC, comment #18: 

I think I've fixed the transpose problem (it looks like it was a typo in the precedence table).

I also fixed the a--' problem.  This was a lexer bug.  It was not allowing ' to be recognized as a transpose operator after -- or ++.  Now it is.  But I think your test was not quite right, so I fixed it.  Instead of


%!  a = [3 5];
%!  assert (2.^a ++, [8 32])
%!  assert (a, [4 6])
%!  assert (a--', [3; 5])
%!  assert (a, [2 4])


I changed it to


%!  a = [3 5];
%!  assert (2.^a ++, [8 32])
%!  assert (a, [4 6])
%!  assert (a--', [4; 6])
%!  assert (a, [3 5])


My current version of the changeset is attached.
Either that, or I think it needs to reinitialize a before doing the --' operation.


(file #23286)

John W. Eaton <jwe>
Group administrator
Thu 21 Apr 2011 07:14:53 PM UTC, comment #17: 

I re-wrote the parser tests in test_parser.m to conform to the new precedence hierarchy.  I'm attaching a diff to this bug report which is your parser changes + my test code.

There are a few failing tests that need addressing.

1) exponent and transpose (the original reporter's issue) is back again.  These two operators are at the same level of precedence and should evaluate from left-to-right.


assert ([2 3].^2.', [4;9])


2) postfix operator and Hermitian conjugate don't work together.  Ordinary transpose does (a--.')


a = [3 5];
assert (a--', [3; 5])



(file #23284)

Rik <rik5>
Group administrator
Wed 20 Apr 2011 02:58:27 AM UTC, comment #16: 

OK, I'm attaching another attempt.  With this, it passes all the tests and the additional one you show below.

(file #23278)

John W. Eaton <jwe>
Group administrator
Wed 20 Apr 2011 02:35:40 AM UTC, comment #15: 

I started trying to write new parser tests against the new documented behavior and came across this:


a = 1;
b = 2.^a++

Under 3.4
b = 2, a = 2

On stable branch w/latest precedence patch
error: invalid lvalue function called in expression


I think the 3.4 behavior is correct since a++ should return 1 to the current expression being evaluated while incrementing the variable a only after the entire expression has been evaluated.

Rik <rik5>
Group administrator
Tue 19 Apr 2011 10:39:24 PM UTC, comment #14: 

I'm attaching the latest changes.  I think this is now a relatively minor change.

The precedence of | has been less than & (and likewise for || and &&) for a long time now.  This is consistent with C and C++, I think, and compatible with Matlab, so there is actually no change there.

I deleted the test for ---a and fixed the one for a^b^c.

The shift operators have not been defined in Octave's scripting language for a long time now, but the parser still recognizes them so it is possible that someone is using them with a user-defined type.  But it seems unlikely enough that I'm willing to just remove them and see if anyone complains.  I'll do that in a separate changeset.

All your parser tests now pass with my current version of the changeset applied.

I think the docs are now correct, and I reordered the table to have highest precedence at the top as you suggested.

(file #23277)

John W. Eaton <jwe>
Group administrator
Tue 19 Apr 2011 06:29:36 PM UTC, comment #13: 


>> I also fixed the test for exponentiation. Matlab docs specifically note that exponentiation is handled left to right. That's odd to me, but I don't see a compelling reason to do things differently.


Agreed.  It's simpler if is just another left-to-right operator.  This sentence in expr.txi should be changed to eliminate the reference to exponentiation: "When operators of equal precedence are used together, the leftmost operator groups first, except for the assignment and exponentiation operators, ..."

>>I also changed the precedence levels of the || and &&, and | and & operators to match Matlab, with the OR operators having lower precedence than the AND operators. Should we try to warn about this change in 3.6?


I don't see the logic in having '|' and '&' at different precedence levels and I don't believe we need to follow Matlab down this path.  But if we do, we will need some tests in test/test_parser.m to check that the newly inserted levels are working correctly.  And yes, we will definitely need to post something to NEWS since a|b&c will now evaluate as a|(b&c) which is the opposite of the existing (a|b)&c interpretation.  Or more likely, an issue will result from using the short-circuit operators.  I would expect a || b && c to first check condition a.  But now, I'm confused because this can be re-interpreted as a || (b && c).  Since parentheses have the highest precedence, does this mean that condition b is checked first?  When using short-circuit operators I try to have the cheap computational test first, but I don't know whether that is a or b in this case.  Eventually I'd probably guess that the parser sees token || token and still evaluates a first, but that's more thinking and guessing then I like to do :)

>>How should we fix the tests for increment and decrement? The test that results in an error for an invalid lvalue should be fixed, I think. The current precedence settings have ++ and -- highest of all except indexing. But what about ---a? That is an incompatibility with Matlab that will be pretty much impossible to fix. OTOH, it is probably not something that comes up too often.


Just scrap the test for ---a.  It's such an obscure corner case and coders should use parentheses if they have any doubt about how the parser will be ordering operations.

>>Speaking of the left and right shift operators (<<, >>), should we remove them? Should we bother deprecating them, or just remove them? There is no actual use of these operators in any Octave scripting code, but users could have defined objects that use them. I don't see any in Octave Forge packages though, so maybe it would be OK to just dump them in 3.6.


What operators are these (<<, >>)?  I just tried

a = 1;
a<<2
error: binary operator `<<' not implemented for `scalar' by `scalar' operations

in versions 3.0.5, 3.2.4, and 3.4.0.  I think they have already been missing from Octave for quite awhile so they can probably just be dumped.

>>I'm attaching the new version of the patch. I'd like to get feedback, but please don't check it in just yet.

No, I'll let you check any patches in.  I would also re-order the operator precedence chart to be from highest to lowest.  It makes more sense for me to see item #1 be the most important.  I would also add parentheses '()' as the first item to make it clear that they trump all other operators.  Finally, the increment and decrement operators are listed as '+','-','++' whereas I think it should be only '++','--'.

Rik <rik5>
Group administrator
Tue 19 Apr 2011 02:46:04 AM UTC, comment #12: 

I'm attaching a new version of the patch.  It makes the precedence levels the same as Matlab, except for the operators that Matlab does not have (OP=, ++/--, and <</>>).

I also fixed the test for exponentiation.  Matlab docs specifically note that exponentiation is handled left to right.  That's odd to me, but I don't see a compelling reason to do things differently.

I also changed the precedence levels of the || and &&, and | and & operators to match Matlab, with the OR operators having lower precedence than the AND operators.  Should we try to warn about this change in 3.6?

How should we fix the tests for increment and decrement?  The test that results in an error for an invalid lvalue should be fixed, I think.  The current precedence settings have ++ and -- highest of all except indexing.  But what about ---a?  That is an incompatibility with Matlab that will be pretty much impossible to fix.  OTOH, it is probably not something that comes up too often.

Speaking of the left and right shift operators (<<, >>), should we remove them?  Should we bother deprecating them, or just remove them?  There is no actual use of these operators in any Octave scripting code, but users could have defined objects that use them.  I don't see any in Octave Forge packages though, so maybe it would be OK to just dump them in 3.6.

I'm attaching the new version of the patch.  I'd like to get feedback, but please don't check it in just yet.

(file #23270)

John W. Eaton <jwe>
Group administrator
Tue 19 Apr 2011 01:42:40 AM UTC, comment #11: 

I think it is a bit risky, so I'd say put it on default.  I would also like to take another look at it, so please let me do that and then I'll commit it.

John W. Eaton <jwe>
Group administrator
Mon 18 Apr 2011 11:26:15 PM UTC, comment #10: 

I was going to commit this patch for operator precedence.  Should it go to 'stable', because this is a bug fix which users should probably see before 3.6.  Or should it go to 'default', because messing with the parser might lead to instabilities and breaking of code that worked before?

Rik <rik5>
Group administrator
Wed 06 Apr 2011 08:18:50 PM UTC, comment #9: 

These are the results Matlab produces:

% Test 1

>> a = 2;
>> ~a++

??? ~a++
        |
Error: Expression or statement is incomplete or incorrect.

(my comment: ++ is not defined in Matlab, so a++ does not work, too)


% Test 2                                                                                                        

>> 2^3^5                                                                                                           


ans =

       32768

(my comment: mathematically wrong left to right evaluation)


% Test 3
a = 2;
---a

ans =

    -2

(my comment: as -- is not defined in Matlab (the same as ++), Matlab's interpretation is: -(-(-a)))

Patrick Häcker <magicmuscleman>
Wed 06 Apr 2011 07:07:26 PM UTC, comment #8: 

Could the original reporter verify the behavior of Matlab for the following tests?


% Test 1
a = 2;
~a++

% Test 2
2^3^5

% Test 3
a = 2;
---a


Rik <rik5>
Group administrator
Fri 18 Mar 2011 03:01:53 PM UTC, comment #7: 

Yes, I'd say go ahead and add the tests.

My time available for working on Octave has been limited lately, but I'll try to take another look at fixing the precedence problems soon.

Thanks.

John W. Eaton <jwe>
Group administrator
Wed 16 Mar 2011 05:15:49 PM UTC, comment #6: 

John, should I add the operator precedence tests to Octave?  They will currently fail for transpose, but I'd like to check them in so I can stop having to track them.

Rik <rik5>
Group administrator
Sat 26 Feb 2011 12:53:07 AM UTC, comment #5: 

I wrote a bunch of tests for operator precedence and have found 3 issues to address.

First, the following:

1) !a++ does not work with the new patch.  Both '!' and '++' are at the same level of precedence, which usually implies left to right evaluation.  However, !a is just a number which then can not be incremented by the '++' operator.  The error message is

error: invalid lvalue function called in expression


2) Exponents, like the assignment operators (=), are supposed to group right-to-left according to the documentation.  This is not the case for either the old parser or the parser with the new patch.  Instead exponents are evaluated left-to-right as any ordinary operator, but contrary to mathematical notions and the documentation.


2^3^5 = 32768 which is 8^5.
2^(3^5) = 1.4e73


3) Unary minus and the pre-decrement operator do not work together.  This one is a real corner case and doesn't necessarily need to be fixed, but I thought I'd mention it.


a = 2;
+--a
ans = 1
---a
error: invalid lvalue function called in expression



Rik <rik5>
Group administrator
Thu 24 Feb 2011 07:59:58 PM UTC, comment #4: 

It's fine with me if the new tests are added to the existing test_parser.m file.

Thanks.

John W. Eaton <jwe>
Group administrator
Thu 24 Feb 2011 06:55:35 PM UTC, comment #3: 

It would be useful to have a set of tests which actually verify that operator precedence rules are respected.  I don't mind writing those, but should they go in the existing test/test_parser.m or in a new file test/test_op_prec.m?

Rik <rik5>
Group administrator
Thu 24 Feb 2011 07:45:09 AM UTC, comment #2: 

The attached patch seems to fix the problem for me.  All the tests still pass without parse errors, so it might be OK, but I have not done any more extensive testing so I'd like to hold off a bit before checking it in.

John W. Eaton <jwe>
Group administrator
Mon 21 Feb 2011 03:41:20 AM UTC, comment #1: 

Confirmed.  Regardless of compatibility, there is a difference between the documented behavior and the actual behavior.  According to the manual, section 8.8 on Operator Precedence, exponentiation has the highest priority of all operations and transpose is 2 levels lower.  Thus, the exponentiation really should happen first followed by the transpose.

I'm going to lower the priority on the issue, however, since there is an obvious work-around of using parentheses to specify the exact order of operations.

Rik <rik5>
Group administrator
Fri 18 Feb 2011 01:58:07 PM UTC, original submission:  

Matlab evaluates the element-wise square and the transpose with equal importance and thus from left to right:

>> size(ones(1,3).^2.')

ans =
     3     1

>> size(ones(1,3).'.^2)

ans =
     3     1


Octave evaluates the transpose first and then the element-wise square and thus transposes the scalar exponent:

octave:1> size(ones(1,3).^2.')
ans =
   1   3

octave:2> size(ones(1,3).'.^2)
ans =
   3   1


I don't know which definition is superior, but I would opt for compatibility.

Patrick Häcker <magicmuscleman>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #23286:  diff6.txt added by jwe (21KiB - text/plain)
file #23284:  diff5.txt added by rik5 (18KiB - text/plain)
file #23278:  diffs.txt added by jwe (11KiB - text/plain)
file #23277:  diffs.txt added by jwe (11KiB - text/plain)
file #23270:  diffs.txt added by jwe (9KiB - text/plain)
file #22771:  diffs.txt added by jwe (7KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by magicmuscleman (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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-04-21 jwe StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2011-04-21 jwe Attached File- Added diff6.txt, #23286
    2011-04-21 rik5 Attached File- Added diff5.txt, #23284
    2011-04-20 jwe Attached File- Added diffs.txt, #23278
    2011-04-19 jwe Attached File- Added diffs.txt, #23277
    2011-04-19 jwe Attached File- Added diffs.txt, #23270
    2011-02-24 jwe Attached File- Added diffs.txt, #22771
    2011-02-24 jwe StatusConfirmed Ready For Test
    2011-02-21 rik5 Priority5 - Normal 3 - Low
        StatusNone Confirmed
        SummaryDifferent operator preference than Matlab Operator precedence for transpose incorrect

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code