bugGNU Octave - Bugs: bug #32885, norm(NaN,Inf) produce 0

 
 

bug #32885: norm(NaN,Inf) produce 0

Submitter:  Sergey B Kirpichev <skirpichev>
Submitted:  Tue 22 Mar 2011 09:33:09 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  lachlan
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 17 Feb 2016 04:05:32 AM UTC, comment #23: 

Thanks, Rik.  I hadn't realized a constructor is so slow.

The other way to avoid the constructor is

 val = std::abs (val)


I wasn't sure how fast testing for NaN is.

BTW, thanks for all the time you spend testing and checking in my patches.  I don't want to clutter your inbox, but I say a little "thank you" each time I get the close notice :)

Lachlan Andrew <lachlan>
Wed 17 Feb 2016 12:03:59 AM UTC, comment #22: 

I used a slightly different fix which avoids calling a constructor.


-    max = std::max (max, std::abs (val));
+    if (xisnan (val))
+      max = octave_NaN;
+    else
+      max = std::max (max, std::abs (val));


Benchmarking on a vector (1, 1e6) with a single NaN value.



Old method: 0.0014278
Rik patch: 0.003131
Lachlan patch: 0.012914


My solution is about 2X slower, but does produce the correct output.

I applied it here (http://hg.savannah.gnu.org/hgweb/octave/rev/85d8280c64f4).  Closing report.

Rik <rik5>
Group administrator
Mon 15 Feb 2016 07:36:59 PM UTC, comment #21: 

Is there a simpler way to achieve the same result?  I benchmarked the patch and it is 10X slower than the existing solution.  Of course, even 10X is rather small.


x = ones (1, 1e6);
x(1,1e5) = NaN;
tic; norm (x, Inf); toc


Old times were 2 ms, new times were ~20 ms.

Rik <rik5>
Group administrator
Thu 03 Dec 2015 06:52:01 AM UTC, comment #20: 

I recommend the somewhat simpler approach of eliminating std::max and std::min entirely, and replacing them with explicit comparisons.  Could someone please review my patch?

Since Matlab has now fixed their issue, we should also fix this.

(file #35623)

Lachlan Andrew <lachlan>
Sun 21 Sep 2014 07:10:55 PM UTC, comment #19: 

It is difficult to follow the comments what "The Matlab (R)" is doing. Probably it has to be noted which version.

Matlab V7.1 R14 SP3 says (9 years old):

>> norm([NaN,0],inf)

ans =
   NaN

>> norm([0,NaN],inf)

ans =
     0

=> This is not plausible, as Marco pointed out. But it was fixed quite some time ago. Maybe "traditional" would go too far.

Matlab R2012a says (2 1/2 years old):

>> norm([NaN,0],inf)

ans =
   NaN

>> norm([0,NaN],inf)

ans =
   NaN

Also see the manual of current R2014a:
http://www.mathworks.de/de/help/matlab/ref/norm.html
"norm returns NaN if the input contains NaN values."

Armin Müller <arm_in>
Sun 21 Sep 2014 03:06:22 PM UTC, comment #18: 

Asking in IRC, it appears Matlab fixed their bug and now returns NaN in both cases.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Sun 21 Sep 2014 09:47:10 AM UTC, comment #17: 

Maybe we should replicate matlabs (IMO stupid) behaviour with --traditional flag given and behave reasonable if its not.

Lasse Schuirmann <sils1297>
Sun 21 Sep 2014 09:37:43 AM UTC, comment #16: 

In relation to comment #5

== octave 3.8.2 ==


octave:3> norm([NaN,0],inf)
ans =                    0
octave:4> norm([0,NaN],inf)
ans =                    0


== matlab ==

Not verified again by me


>> norm([NaN,0],inf)
ans =
   NaN
>> norm([0,NaN],inf)
ans =
     0


== With the Patch from Lasse ==


octave:2> norm([NaN,0],inf)
ans = NaN
octave:3> norm([0,NaN],inf)
ans = NaN


So the assert tests are passed. And as far as I see, we got now all variants, but not what Matlab did.

The real question is, how should GNU Octave behave?

Markus Bergholz <markuman>
Tue 14 Jan 2014 08:07:06 PM UTC, comment #15: 

I have learned that the patches I provided were not in the right shape. So here's the full patch in the right format.

(file #30274)

Lasse Schuirmann <sils1297>
Sat 30 Nov 2013 12:50:19 PM UTC, comment #14: 

Hello again,

to make the patch testing easier and ensure that this bug doesn't occur again I wrote a test. For performance reasons I also inlined the function. Besides the two inline keywords, a few comments and the test this patch doesn't do anything!

So just invoke make check (or the test manually) to check if this works. (Next time I plan include the tests with the first patch.)

Greetings,

Lasse Schuirmann



(file #29741)

Lasse Schuirmann <sils1297>
Fri 29 Nov 2013 05:00:39 PM UTC, comment #13: 

Hello Everyone,

I searched a bit about why std::min and std::max return 0 for (0, NaN). (Yes, the min function is affected too.) More about this here. [1]

I think that whithin a tool like octave this should either give a warning or return NaN. I do not understand the behaviour of matlab in this case.

I attached a patch for this bug which returns NaN for the specific scenarios. (Since this is my first one I hope that it's alright; feel free to send any recommendations to me)

It is not the most performant one but it works on my machine. (One could implement an abortion of the whole loop if one result is NaN and would save all these loops.)

Greetings,

Lasse Schuirmann
-email is unavailable-

[1] http://en.wikipedia.org/wiki/IEEE_754_revision#min_and_max

(file #29737)

Lasse Schuirmann <sils1297>
Tue 05 Apr 2011 02:38:42 PM UTC, comment #12: 

Hi Sean, thanks for the suggestion!  But I don't think this is really something where it makes sense to work together -- I imagine the fix should not be too complicated.  So just do it yourself and I'll experiment with other stuff -- this will probably also benefit Octave more, since then two bugs get fixed :D

Daniel Kraft <domob>
Tue 05 Apr 2011 02:34:57 PM UTC, comment #11: 


> I haven't started working on this, so my status is to leave it to Sean.


Hi Daniel, I think we could probably work on things together, it'd be a good learning experience for both of us. I have a dedicated host at my uni so I can give you a shell account and the like. How does that sound to you?

Sean Young <seannz>
Tue 05 Apr 2011 02:32:50 PM UTC, comment #10: 

I haven't started working on this, so my status is to leave it to Sean.

Daniel Kraft <domob>
Tue 05 Apr 2011 02:28:51 PM UTC, comment #9: 

Daniel, Sean, I'm marking this bug as "in progress" since both of you have said you're working on it. The right fix should not be difficult, so I have hope one of you can produce it soon.

Daniel, I believe Sean has been working on this bug for a little while longer. We have been discussing this over IRC, in Freenode's #octave.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Tue 05 Apr 2011 01:52:36 AM UTC, comment #8: 


> Then I'll try to create a patch for this approach.


Hi Daniel, work on this bug is already under way, I will be submitting a patch to this bug shortly.

Sean Young <seannz>
Mon 04 Apr 2011 09:55:58 AM UTC, comment #7: 

I would like to tackle this as kind of "first patch" (seems appropriate).  So is the consensus that we should implement it "the safe way" via a special check?

Then I'll try to create a patch for this approach.

Daniel Kraft <domob>
Thu 24 Mar 2011 12:52:48 PM UTC, comment #6: 


> So as I see it, this is a corner case, but my patch seems to work on major compilers.


e.g. icc ?

> We could introduce the "right" patch that explicitly checks for
> NaN, at the risk of introducing some inefficiency... or we could use this
> patch which is likely to work for a long time on all major compilers.


I hope, we should do the first option.  Moreover, this fix doesn't solve

>> norm([0,NaN],inf)
ans = 0

I think we shouldn't clone stupid things that do Matlab.

Sergey B Kirpichev <skirpichev>
Thu 24 Mar 2011 08:00:27 AM UTC, comment #5: 

Without your patch, I see


> norm([NaN,0],inf)
ans = 0
> norm([0,NaN],inf)
ans = 0


with your patch


> norm([NaN,0],inf)
ans = 0
> norm([0,NaN],inf)
ans = NaN


in Matlab


>> norm([NaN,0],inf)
ans =
   NaN
>> norm([0,NaN],inf)
ans =
     0


None of them are satisfactory, but I agree with you that at least your patch gives norm(NaN,inf)=NaN (as Matlab) and therefore should be applied.

Marco

Marco Caliari <caliari>
Group Member
Thu 24 Mar 2011 05:20:19 AM UTC, comment #4: 

So as I see it, this is a corner case, but my patch seems to work on major compilers. We could introduce the "right" patch that explicitly checks for NaN, at the risk of introducing some inefficiency... or we could use this patch which is likely to work for a long time on all major compilers.

My preference is for the latter option.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Wed 23 Mar 2011 12:56:10 PM UTC, comment #3: 

For me too (g++ (Debian 4.4.5-8) 4.4.5), but it's not seems to be safe.  For example, MSDN says that min/max returns the first element if neither is greater/lesser.  But in general it's undefined, see e.g.:
http://www.tech-archive.net/Archive/VC/microsoft.public.vc.stl/2007-11/msg00023.html

Sergey B Kirpichev <skirpichev>
Wed 23 Mar 2011 09:05:51 AM UTC, comment #2: 

Your patch fixes the problem for me too (octave 3.4.0 compiled with gcc 4.3.3).

Marco

Marco Caliari <caliari>
Group Member
Wed 23 Mar 2011 05:43:33 AM UTC, comment #1: 

I've isolated the problem to the following code snippet in gcc 4.4.5:


#include <iostream>

int main()
{
  using namespace std;
  cout << max(0.0, 0.0/0.0) << " "
       << max(0.0/0.0, 0.0) << endl;
}


On my system, that outputs "0 -nan", i.e. the order in which NaN is passed to max matters. I attach a patch that works around this for gcc 4.4.5, but I am not sure this is the right fix in general; I can imagine how it depends on the implementation of std::max.

I'll let someone else push this patch if they deem it worthy.

(file #22992)

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Tue 22 Mar 2011 09:33:09 PM UTC, original submission:  


>> norm(1,inf)
ans =  1
>> norm(NaN(2),Inf)
ans = NaN
>> norm(NaN,Inf) % or norm(NaN(1),Inf)
ans = 0

I think, the last result should be NaN.  (Same holds for OPT=-Inf.)

Sergey B Kirpichev <skirpichev>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #35623:  bug_32885-NaNnorm.patch added by lachlan (1KiB - text/x-diff)
file #30274:  32885_complete.patch added by sils1297 (4KiB - text/x-patch - Complete patch.)
file #29741:  32885_tests.diff added by sils1297 (2KiB - text/x-patch - This file does not contain the patch itself but some minor changes, comments and testing!)
file #29737:  patch32885.diff added by sils1297 (3KiB - text/x-patch)
file #22992:  NaN.patch added by jordigh (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by arm_in (Posted a comment)
  • -email is unavailable- added by markuman (Posted a comment)
  • -email is unavailable- added by sils1297
  • -email is unavailable- added by sils1297 (Updated the item)
  • -email is unavailable- added by seannz (Posted a comment)
  • -email is unavailable- added by domob (Posted a comment)
  • -email is unavailable- added by caliari (Posted a comment)
  • -email is unavailable- added by jordigh (Updated the item)
  • -email is unavailable- added by skirpichev (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 17 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-02-17 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2016-02-10 lachlan Assigned toNone lachlan
    2016-02-08 lachlan StatusPatch Submitted Ready For Test
    2015-12-03 lachlan Attached File- Added bug_32885-NaNnorm.patch, #35623
    2014-01-14 sils1297 Attached File- Added 32885_complete.patch, #30274
        Carbon-Copy- Added -email is unavailable-
    2013-11-30 sils1297 Attached File- Added 32885_tests.diff, #29741
    2013-11-29 jordigh StatusConfirmed Patch Submitted
    2013-11-29 sils1297 Attached File- Added patch32885.diff, #29737
    2012-06-28 jordigh StatusIn Progress Confirmed
        Assigned tojordigh None
        Release3.4.0 dev
    2011-04-15 jordigh Assigned toNone jordigh
    2011-04-05 jordigh StatusPatch Submitted In Progress
    2011-03-23 jordigh Attached File- Added NaN.patch, #22992
        StatusNone Patch Submitted

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code