Add `:float` support #1055

seanstrom · 2024-05-10T11:44:38Z

Summary

This PR attempts to add :float support as described in There's no :float #722
This PR is a follow up from the original attempt to add float support in Add :float type schema #725

Notes

I've tried to add float support to Malli while being mindful of the potentially different meanings of float between Clojure and ClojureScript. Specifically, I've tried to make Malli to treat floats as Java/Clojure floats (instances of Float) while using clj, and fallback to doubles while using cljs

… namespaces

seanstrom

Self-review comments 📖

seanstrom · 2024-05-10T11:47:18Z

src/malli/generator.cljc

+(defmethod -schema-generator :float [schema options]
+ (let [max-float #?(:clj Float/MAX_VALUE :cljs (.-MAX_VALUE js/Number))
+ min-float #?(:clj Float/MIN_VALUE :cljs (.-MIN_VALUE js/Number))
+ ceil-max (fn [v] (if (nil? v) max-float (min max-float v)))
+ floor-min (fn [v] (if (nil? v) min-float (max min-float v)))
+ props (m/properties schema options)
+ min-max-props (-min-max schema options)
+ infinite? (get props :gen/infinite? false)
+ NaN? (get props :gen/NaN? false)]
+ (gen/double* (merge {:infinite? infinite?
+ :NaN? NaN?}
+ (when (or (not infinite?) NaN?)
+ {:min (floor-min (:min min-max-props))
+ :max (ceil-max (:max min-max-props))})))))


Here I was unsure how to exactly generate a float or a instance of Float, but I attempted to constrain the generated values to something that could be coerced into a Float instance.

If someone knows how/where to coerce the generated value into a Float instance then please let me know 🙏

Call (gen/fmap float (gen/double* ...)).

Although that doesn't work for ##Inf and ##-Inf. Might not make sense to allow them in a float generator, at least on the jvm.

Call (gen/fmap float (gen/double* ...)).

Ah perfect thank you for this 🙏

Although that doesn't work for ##Inf and ##-Inf. Might not make sense to allow them in a float generator, at least on the jvm.

Yeah that makes sense, I'll try setting infinite? to always be false.

seanstrom · 2024-05-10T11:48:49Z

src/malli/transform.cljc

+(defn parse-float [s]
+ #?(:clj
+ (if (string? s)
+ (try
+ (Float/parseFloat s)
+ (catch NumberFormatException _ nil))
+ (throw (IllegalArgumentException.
+ (str "Expected string, got " (if (nil? s) "nil" (-> s class .getName))))))
+ :cljs
+ (parse-double s)))
+
+(defn -string->float [x]
+ (if (string? x)
+ (or (parse-float x) x)
+ x))
+


Here I'm providing a custom definition for parsing floats from strings. I could not find a function that used Float/parseFloat in this way, so I copied and modified a similar function from Clojure source code.

seanstrom · 2024-05-10T11:52:24Z

test/malli/generator_test.cljc

+
+ (testing "float properties"
+ (let [infinity? #(or (= % ##Inf)
+ (= % ##-Inf))
+ NaN? (fn [x]
+ (#?(:clj (fn [v]
+ (or (infinity? v) (Float/isNaN v)))
+ :cljs js/isNaN)
+ x))
+ special? #(or (NaN? %)
+ (infinity? %))
+ test-presence (fn [f options]
+ (some f (mg/sample [:float options]
+ {:size 1000})))]
+ (is (test-presence infinity? {:gen/infinite? true}))
+ (is (test-presence NaN? {:gen/NaN? true}))
+ (is (test-presence special? {:gen/infinite? true
+ :gen/NaN? true}))
+ (is (not (test-presence special? nil)))))


It was a bit tricky writing this test since I want to use Float/isNaN, but sometimes when generating Infinity the tests would error because of the value being too big to coerce into a float.

Note, I don't think this test generates Float instances, so I think it should be updated to check for that but I'm unsure how to do that. If someone know how please let me know 🙏

frenchy64 · 2024-05-15T03:20:26Z

src/malli/generator.cljc

@@ -454,6 +454,20 @@
 {:infinite? (get props :gen/infinite? false)
 :NaN? (get props :gen/NaN? false)})
 (-min-max schema options))))
+(defmethod -schema-generator :float [schema options]
+ (let [max-float #?(:clj Float/MAX_VALUE :cljs (.-MAX_VALUE js/Number))
+ min-float #?(:clj Float/MIN_VALUE :cljs (.-MIN_VALUE js/Number))


I don't think Float/MIN_VALUE is the minimum float, I think it's (- Float/MAX_VALUE).

Ah yes good catch 🙏

I just meant for JVM, (.-MIN_VALUE js/Number) seems appropriate for cljs.

Do we want the min-float to be a negative number? Because I think (.-MIN_VALUE js/Number) would be the smallest positive number.

…ing floats

frenchy64 · 2024-05-17T16:33:39Z

Oh I see, I misread the docs. Do we even need to specify the bounds on cljs since it looks like the same as double? -------- Original Message --------

…

On May 17, 2024, 05:44, Sean Hagstrom wrote: @seanstrom commented on this pull request. --------------------------------------------------------------- In [src/malli/generator.cljc](#1055 (comment)): > @@ -454,6 +454,20 @@ {:infinite? (get props :gen/infinite? false) :NaN? (get props :gen/NaN? false)}) (-min-max schema options)))) +(defmethod -schema-generator :float [schema options] + (let [max-float #?(:clj Float/MAX_VALUE :cljs (.-MAX_VALUE js/Number)) + min-float #?(:clj Float/MIN_VALUE :cljs (.-MIN_VALUE js/Number)) Do we want the min-float to be a negative number? Because I think (.-MIN_VALUE js/Number) would be [the smallest positive number](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MIN_VALUE#description). — Reply to this email directly, [view it on GitHub](#1055 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AACGFJF3HTOJIGOJDQ7BWMLZCXNRRAVCNFSM6AAAAABHQPRGQGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRSHE4DENJXGY). You are receiving this because you commented.Message ID: ***@***.***>

seanstrom · 2024-05-18T09:30:59Z

Do we even need to specify the bounds on cljs since it looks like the same as double?

Yeah good point, we probably don't need to do it, so I've changed it to only set the bounds for the clj and allow cljs to behave as doubles. I've also made sure to allow for infinity to be generated for cljs too.

frenchy64 · 2024-05-20T01:31:24Z

src/malli/generator.cljc

+ min-max-props (-min-max schema options)
+ min-max-clamped #?(:clj {:min (floor-min (:min min-max-props))
+ :max (ceil-max (:max min-max-props))}
+ :cljs min-max-props)


I can't think of a compelling reason to clamp the bounds, perhaps an error should be thrown instead.

Relatedly, the bounds should be coerced to float so then floating point numbers are generated regardless of bounds like in this PR #1052

I can't think of a compelling reason to clamp the bounds, perhaps an error should be thrown instead.

Well the main reason I thought to clamp the bounds is because we're using gen/double*, which can produce numbers that will lead to thrown exceptions when trying to coerce them into floats. So I think that means we always need to clamp the bounds, but maybe we can also throw an error if someone tries to override those boundaries (?).

the bounds should be coerced to float so then floating point numbers are generated regardless of bounds

I'll push up a commit to coerce the bounds to floats too 👍

seanstrom added 3 commits May 10, 2024 12:37

feature: add support for :float in core, json-schema, and transform…

e1ca006

… namespaces

tweak: add generators and generator tests for float

d4ee0c7

tweak: add support for :float for providers

6c87f47

seanstrom commented May 10, 2024

View reviewed changes

frenchy64 reviewed May 15, 2024

View reviewed changes

seanstrom added 6 commits May 16, 2024 11:11

fix: ensure we use the negated max-float for lower bound when generat…

52d54c9

…ing floats

tweak: coerce generated doubles into floats

87e0317

tweak: check for the absence of infinity when generating floats

f58cb50

tweak: check for float instance when testing generation of floats

3c83de8

tidy: delete whitespace

24987d9

tweak: configure float generator to not generate infinity

19dc5ae

seanstrom requested a review from frenchy64 May 16, 2024 14:16

seanstrom added 3 commits May 18, 2024 10:20

tweak: only clamp float generation for jvm runtime

6076934

tidy: simplify NaN check for generator testing

8a0c7d3

tweak: allow for infinity when generating floats on cljs

7d9ff4f

frenchy64 reviewed May 20, 2024

View reviewed changes

tweak: coerce min and max to floats

9a99ef2

seanstrom requested a review from frenchy64 May 23, 2024 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `:float` support #1055

Add `:float` support #1055

seanstrom commented May 10, 2024

seanstrom left a comment •

edited

seanstrom May 10, 2024

frenchy64 May 15, 2024

frenchy64 May 15, 2024

seanstrom May 16, 2024

seanstrom May 10, 2024

seanstrom May 10, 2024

frenchy64 May 15, 2024

seanstrom May 16, 2024

frenchy64 May 17, 2024

seanstrom May 17, 2024

frenchy64 commented May 17, 2024 via email

seanstrom commented May 18, 2024

frenchy64 May 20, 2024

seanstrom May 20, 2024

Add :float support #1055

Are you sure you want to change the base?

Add :float support #1055

Conversation

seanstrom commented May 10, 2024

Summary

Notes

seanstrom left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenchy64 commented May 17, 2024 via email

seanstrom commented May 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `:float` support #1055

Add `:float` support #1055

seanstrom left a comment •

edited