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
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 :)
|
Wed 17 Feb 2016 12:03:59 AM UTC, comment #22:
I used a slightly different fix which avoids calling a constructor.
Benchmarking on a vector (1, 1e6) with a single NaN value.
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.
|
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.
Old times were 2 ms, new times were ~20 ms.
|
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)
|
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."
|
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.
|
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.
|
Sun 21 Sep 2014 09:37:43 AM UTC, comment #16:
In relation to comment #5
== octave 3.8.2 ==
== matlab ==
Not verified again by me
== With the Patch from Lasse ==
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?
|
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)
|
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)
|
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
-unavailable-
[1] http://en.wikipedia.org/wiki/IEEE_754_revision#min_and_max
(file #29737)
|
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
|
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?
|
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.
|
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.
|
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.
|
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.
|
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
I think we shouldn't clone stupid things that do Matlab.
|
Thu 24 Mar 2011 08:00:27 AM UTC, comment #5:
Without your patch, I see
with your patch
in Matlab
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
|
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.
|
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
|
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
|
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:
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)
|
Tue 22 Mar 2011 09:33:09 PM UTC, original submission:
I think, the last result should be NaN. (Same holds for OPT=-Inf.)
|