patchGNU Octave - Patches: patch #9065, octave::math::min and max template...

 
 

patch #9065: octave::math::min and max template ignore isnan

Submitter:  Carnë Draug <carandraug>
Submitted:  Tue 02 Aug 2016 03:29:36 AM UTC
   
 
Category:  Core : other Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Tue 02 Aug 2016 12:42:18 PM UTC, comment #4: 


> Why do you need or care about long double? I omitted those because they were not used in Octave, nor do I expect them to be.


If liboctave is not meant to be used with types other than double, float, and octave_int's then this is not a problem (and this patch only reduces a bit of code duplication).

But aren't we moving in the direction of making this a C++ library more useful by other projects? I think should be considering other types and defining this operations better so it's clear what min and max are meant to compute for types we don't know about, and the requirements on those types.

Carnë Draug <carandraug>
Group Member
Tue 02 Aug 2016 06:51:42 AM UTC, comment #3: 

Why do you need or care about long double?  I omitted those because they were not used in Octave, nor do I expect them to be.

John W. Eaton <jwe>
Group administrator
Tue 02 Aug 2016 06:48:01 AM UTC, comment #2: 

We have octave_int so that we can provide the saturation semantics for integers that is required for Matlab compatibility.

John W. Eaton <jwe>
Group administrator
Tue 02 Aug 2016 04:35:04 AM UTC, comment #1: 

Draft proposal at https://bitbucket.org/carandraug/octave/commits/e01c4646def1cb06518c6565744e83b601fcb61a

Note that I choose to go with std::max and std::min instead of writing it ourselves for the octave_int specializations (not sure if that could cause performance issues).

Carnë Draug <carandraug>
Group Member
Tue 02 Aug 2016 03:29:36 AM UTC, original submission:  

Octaves definition of min is the smallest of the non NaN values, but liboctave's templated octave::math::min function does not behave that way. Instead, it behaves like std::min and we have a specialization for double and float.

This is confusing because: 1) the template behaviour is different from Octave, 2) the specialization actually changes the english definition of Octave's min (given another type, other than double and low, with a concept of nan, min would not consider the nan value).  An actual example:


  long double x = 5;
  long double y = NAN;
  double r = octave::math::min (x, y); // incorrectly returns NAN


I tried to change this so that the general template does check for nan, while keeping the specialization for octave_int.

This caused problems with int and short because they are missing isnan. These were required because Octave declares MArray<int> and MArray<short> so I defined isnan for int and short (although I'm not sure why, MArray<int> and MArray<short>, are not used anywhere and we already have Marray for all the octave_int --- or why do we have octave_int at all?).

Carnë Draug <carandraug>
Group Member

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

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 carandraug (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 logged-in users can vote.

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-03-07 mtmiller CategoryNone Core : other

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code