Project

General

Profile

Actions

Bug #18187

closed

Float#clamp() returns ArgumentError (comparison of Float with 1 failed)

Added by SouravGoswami (Sourav Goswami) over 2 years ago. Updated over 2 years ago.

Status:
Rejected
Assignee:
-
Target version:
-
ruby -v:
ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux]
[ruby-core:105377]

Description

When I have a Float::NAN as a number, I expect all the method to work properly.

For example, Float::NAN - 1 gives NAN. But Float::NAN.to_i raises FloatDomainError.

But in case of clamp(), Float::NAN.clamp(0, 100) returns ArgumentError (comparison of Float with 1 failed)

This error doesn't explain what's actually wrong. I didn't write the comparison to compare Float with 1. I didn't pass any invalid argument either. This error is a reflection of what's going on in the C level, which shouldn't appear to the user.

If I write a vanilla clamp() in ruby:

Float.define_method(:clamp2) { |min, max| self < min ? min : self > max ? max : self }

In this case, I can call it like this:

> 8.0.clamp2(10, 100)
=> 10
> 80.0.clamp2(10, 100)
=> 80.0
> 800.0.clamp2(10, 100)
=> 100
> Float::NAN.clamp2(10, 100)
=> NaN

As you can see, it just returns NAN. But in case of the built-in clamp, it raises the ArgumentError, even though my arguments are just correct. So this should handle this clamp() correctly, either returning the min value or Float::NAN.

Updated by mame (Yusuke Endoh) over 2 years ago

On my machine, the code raises comparison of Float with 0 failed, instead of ... with 1 failed. I have no idea where 1 comes from.

$ ruby -ve 'Float::NAN.clamp(0, 100)'
ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux]
-e:1:in `clamp': comparison of Float with 0 failed (ArgumentError)
        from -e:1:in `<main>'

BTW, I have no opinion about what Float::NAN.clamp(0, 100) should return or raise.

Updated by SouravGoswami (Sourav Goswami) over 2 years ago

Hi, sorry, yes it's comparison of Float with 0 failed, probably there was some typo.

I agree it should raise or return. But it shouldn't raise ArgumentError. Anyway, it probably has the lowest priority because it doesn't cause any issue so far.

The best would be returning Float::NAN? Because that's the behaviour you get when you write comparison in Ruby.

Apparently it will waste some CPU time on isnan(RFLOAT_VALUE(self)) though...

Updated by jeremyevans0 (Jeremy Evans) over 2 years ago

I submitted a pull request to make Float::NAN.clamp return the receiver: https://github.com/ruby/ruby/pull/4884

However, like @mame (Yusuke Endoh), I'm not sure if it is more desirable to raise in this case.

Updated by mrkn (Kenta Murata) over 2 years ago

I think it's OK to return NaN for all the cases of Float::NAN.clamp.

Updated by nobu (Nobuyoshi Nakada) over 2 years ago

What about Float#clamp?

diff --git i/numeric.c w/numeric.c
index db2b2eb2793..12edb0f6006 100644
--- i/numeric.c
+++ w/numeric.c
@@ -2844,6 +2844,13 @@ num_step(int argc, VALUE *argv, VALUE from)
     return from;
 }
 
+static VALUE
+flo_clamp(int argc, VALUE *argv, VALUE x)
+{
+    if (isnan(RFLOAT_VALUE(x))) return x;
+    return rb_call_super(argc, argv);
+}
+
 static char *
 out_of_range_float(char (*pbuf)[24], VALUE val)
 {
@@ -5789,6 +5796,7 @@ Init_Numeric(void)
     rb_define_method(rb_cFloat, "finite?",   rb_flo_is_finite_p, 0);
     rb_define_method(rb_cFloat, "next_float", flo_next_float, 0);
     rb_define_method(rb_cFloat, "prev_float", flo_prev_float, 0);
+    rb_define_method(rb_cFloat, "clamp", flo_clamp, -1);
 }
 
 #undef rb_float_value

Updated by matz (Yukihiro Matsumoto) over 2 years ago

I vote for keeping NaN raises exceptions.

Matz.

Actions #7

Updated by jeremyevans0 (Jeremy Evans) over 2 years ago

  • Status changed from Open to Rejected
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0