Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add :float support #1055

Merged
merged 15 commits into from
Jun 29, 2024
Merged

Add :float support #1055

merged 15 commits into from
Jun 29, 2024

Conversation

seanstrom
Copy link
Contributor

Summary

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

Copy link
Contributor Author

@seanstrom seanstrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review comments 📖

src/malli/generator.cljc Outdated Show resolved Hide resolved
Comment on lines +70 to +85
(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))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

test/malli/generator_test.cljc Outdated Show resolved Hide resolved
src/malli/generator.cljc Outdated Show resolved Hide resolved
@seanstrom seanstrom requested a review from frenchy64 May 16, 2024 14:16
@frenchy64
Copy link
Contributor

frenchy64 commented May 17, 2024 via email

@seanstrom
Copy link
Contributor Author

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.

src/malli/generator.cljc Outdated Show resolved Hide resolved
@seanstrom seanstrom requested a review from frenchy64 May 23, 2024 09:02
@ikitommi
Copy link
Member

ikitommi commented Jun 6, 2024

Would like to merge this, but not 100% sure if everything is resolved. What do you think @frenchy64 ?

@frenchy64
Copy link
Contributor

IMO it's a bit inconsistent with the rest of malli that the bounds are clamped on behalf of the user in the generator. Otherwise the rest looks good.

@seanstrom
Copy link
Contributor Author

Happy to make more changes to this PR, though I'm not sure if I understand the suggestions. I mentioned in an earlier comment that there was a reason to clamp the bounds by default when generating floats. The main reason is that calling (fmap float) after using gen/double* can accidentally create exceptions when coercing the 64-bit double into a 32-bit float.

Here's an example error message:

Exception: java.lang.IllegalArgumentException: Value out of range for float: -2.631375492124477E72

To avoid these exceptions I've included the default clamping of the bounds, because it would seem unstable to accidentally generate exceptions in test suites. There may be another way of avoiding exceptions, if so let's discuss some alternatives.

cc @ikitommi @frenchy64

@seanstrom
Copy link
Contributor Author

One thing to note, I've recently seen this documentation on MDN about creating 32-bit floats with Math.fround in JavaScript. Some interesting things came to mind that could help:

  • Maybe we could support generating 32-bit floats for ClojureScript using Math.fround?
  • Math.fround rounds a 64-bit float into 32-bit float, maybe we could do something similar in Clojure/Java?
  • Math.fround treats numbers outside the range of the minimum and maximum 32-bit float as Infinity and -Infinity, so perhaps we could clamp the bounds by returning Infinity or -Infinity?

@seanstrom
Copy link
Contributor Author

seanstrom commented Jun 11, 2024

Based on some Java documentation, it seems we can use the .floatValue method on a double in Java. This seems somewhat similar to Math.fround because it reduces the data of the 64-bit float into a 32-bit float. It also seems to return Infinity and -Infinity for numbers that are out of the minimum and maximum range.

Based on that these two functions behave similarly and support 32-bit floats on both CLJ and CLJS, I'll try moving forward with using both of these techniques when generating :floats in Malli.

@frenchy64
Copy link
Contributor

frenchy64 commented Jun 11, 2024 via email

@seanstrom
Copy link
Contributor Author

seanstrom commented Jun 11, 2024

Are there other examples of truncation in malli that you are following?

I suppose my thought process is around: "what does it mean to generate a float"?
I can understand that we don't want to clamp boundaries and override the users intent, but by default I think we should always generate valid 32-bit floats when the user doesn't specify a boundary. Otherwise the generator, built upon double*, could generate an invalid 32-bit float and throw an exception.

I would like to avoid throwing exceptions by default, which is what happens now if we don't include the clamped boundaries. The test suite for running the malli float generator will create errors because of the thrown exceptions.

I could specify the boundaries in the tests when using the float generator, but that would mean each time we go to use the float generator we would need to specify the default range of 32-bit floats. And that would seem tedious since we know the default range of a 32-bit float.

To my taste this should mirror bounding a double with a bigint, what happens there? Or a hypothetical bytes generator.

Are you asking what would happen if we try to coerce a BigInt into a 64-bit double? If so, the behaviour would be to create a double that has as much precision as it can allow, or otherwise return Infinity or -Infinity. Here's some related documentation: https://www.javatpoint.com/java-biginteger-doublevalue-method

I think it would be nice for the distribution of the generator to correspond to the bounds provided by the user and for the generator to always return a float. If the distribution is not possible like lower bound being too small or even nonsensical like between Double/MIN_VALUE and Double/MIN_VALUE+1, throw.

If someone inputs min/max boundaries that are outside the range/magnitude of the 32-bit float then sure we can decide to throw. However, I don't think we should throw by default when the boundaries are not specified.


Based on your notes, I think we can agree that we should throw when the user provides boundaries that are unsupported by 32-bit floats. Can we also agree that we don't want to throw exceptions when no boundaries are provided to the float generator?

@frenchy64
Copy link
Contributor

Based on your notes, I think we can agree that we should throw when the user provides boundaries that are unsupported by 32-bit floats. Can we also agree that we don't want to throw exceptions when no boundaries are provided to the float generator?

Exactly.

@seanstrom
Copy link
Contributor Author

seanstrom commented Jun 11, 2024

Okay great! I've pushed up a commit with that change, when you have a moment can you confirm that it's okay please 🙏

src/malli/generator.cljc Outdated Show resolved Hide resolved
@ikitommi ikitommi merged commit c22d377 into metosin:master Jun 29, 2024
9 checks passed
@ikitommi
Copy link
Member

Thanks @seanstrom for doing and @frenchy64 for the review 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants