bugGNU Octave - Bugs: bug #52380, lower/upper functions should pass...

 
 

bug #52380: lower/upper functions should pass any numeric value?

Submitter:  A.R. Burgers <arb>
Submitted:  Thu 09 Nov 2017 10:08:51 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 11 Jan 2018 08:52:57 PM UTC, comment #6: 

All done.  I checked in your changeset here (http://hg.savannah.gnu.org/hgweb/octave/rev/6ece7c7e4912).  Thanks for the code!

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Wed 10 Jan 2018 11:04:30 PM UTC, comment #5: 

Thanks Rik.  I'll make sure to make tests in the future.  In the mean time, I've updated the patch you made to include the update to the missing file.

(file #42885)

Stevie Schraudner <stevietodd>
Fri 05 Jan 2018 06:29:57 AM UTC, comment #4: 

@Stevie: I modified your changeset slightly.  Octave follows the GNU coding guidelines for commit messages which are a one-line summary (which you had) followed by a per file/per function explanation of what was changed.  I added the second bit.  We also try, when we're being good, to use test-driven development.  So I wrote BIST tests for toupper and tolower and included them in mappers.cc (where the two functions reside).  I then compiled and ran


octave:1> test mappers.cc
***** assert (tolower (single (68i)), single (68i))
!!!!! test failed
tolower: not defined for float complex scalar


which proves why tests are important.  Apparently the float scalar case in ov-flt-complex.cc is missing.

I uploaded the most recent version of your cset so you can patch it to work for all possible inputs.


(file #42841)

Rik <rik5>
Group administrator
Wed 27 Dec 2017 02:52:35 PM UTC, comment #3: 

I'm new to contributing so I'm not sure if I'm following the process correctly, but I have attached a changeset at the bottom that should fix this bug.  Thanks.

(file #42738)

Stevie Schraudner <stevietodd>
Mon 13 Nov 2017 07:17:37 PM UTC, comment #2: 

from an examination of the code, it appears making lower/upper work on complex numbers entails implementing the umap_xtolower/umap_xtoupper to files ov-cx-mat.cc, ov-flt-cx-mat.cc and complex.cc in libinterp/octave-value.

A.R. Burgers <arb>
Sat 11 Nov 2017 01:08:02 AM UTC, comment #1: 

I can confirm this for R2017b, even though the Matlab documentation does not mention this use case:

https://www.mathworks.com/help/matlab/ref/lower.html

For what I see, this item is independent of bug #52358, but it is an example where this behavior was triggered.

Kai Torben Ohlhus <siko1056>
Group Member
Thu 09 Nov 2017 10:08:51 PM UTC, original submission:  

See http://savannah.gnu.org/bugs/?52358

There it appears tolower fails on complex numeric values.
I guess lower/upper should return any numeric value unchanged:

On matlab

>> version
ans = 8.6.0.267246 (R2015b)

>> lower(rand(2))
ans =
    0.8147    0.1270
    0.9058    0.9134

>> lower(i)
ans =  0.0000 + 1.0000i


octave:


octave:1> version
ans = 4.3.0+

octave:2> lower(rand(2))
ans =
   0.75904   0.20647
   0.35655   0.51602

octave:3> lower(i)
error: tolower: not defined for complex scalar


A.R. Burgers <arb>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #42885:  52380.cset added by stevietodd (4KiB - application/octet-stream - Thanks RIk. I'll make sure to add tests next time. For now, I've updated your patch to include the update to the missing file.)
file #42841:  52380.cset added by rik5 (4KiB - application/octet-stream)
file #42738:  mypatch.patch added by stevietodd (2KiB - text/x-patch - Changeset to fix this bug)
file #42737:  mypatch.patch added by stevietodd (2KiB - text/x-patch - Changeset to fix this bug)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by stevietodd (Updated the item)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by arb (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-01-11 rik5 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2018-01-10 stevietodd Attached File- Added 52380.cset, #42885
    2018-01-05 rik5 Attached File- Added 52380.cset, #42841
        StatusConfirmed Patch Reviewed
    2017-12-27 stevietodd Attached File- Added mypatch.patch, #42738
    2017-12-27 stevietodd Attached File- Added mypatch.patch, #42737
    2017-11-11 siko1056 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code