Improve precision of float multiple_of validation

[K-dash]

Change Summary

Improved precision of float multiple_of validation and added test cases.

• Refine the validation logic for better handling of floating-point rounding errors

  • Implement a more robust comparison using both absolute and relative error thresholds

• Expand test cases to cover a wider range of scenarios

  • Add comprehensive tests for multiples of 0.5, 0.1, 2.0, and 0.01

• Add identical tests for decimal multiple_of to ensure consistency with float

  • pydantic-core/tests/validators/test_decimal.py::test_decimal_multiple_of

Related issue number

fix pydantic/pydantic#9871

Checklist

• Unit tests for the changes exist
• Documentation reflects the changes where applicable
• Pydantic tests pass with this pydantic-core (except for expected changes)
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @samuelcolvin

[K-dash]

please review

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

[codspeed-hq]

CodSpeed Performance Report

Merging #1373 will not alter performance

_{Comparing K-dash:improve-float-multiple-of-validation (8e9b338) with main (cd2521e)}

Summary

✅ 155 untouched benchmarks

[sydney-runkle]

Thanks for your work on this!

I think we should wait for a review from @samuelcolvin or @davidhewitt on this one. They're well versed regarding the nitty gritty details of floating point ops in rust, and I think they could offer valuable feedback on this new approach.

I left one comment - I'd prefer that we leave all existing tests + add new ones as well, rather than removing some existing tests.

[davidhewitt]

I'm ok with changing this, but I think before we accept this PR we should discuss a bit what we want multiple_of for floats to do (and if we already document any behaviour).

Really, I think users expect this to match Python, so I assume that broadly we want the Python expression value % multiple_of == 0 to be true in Python for all value and multiple_of which we validate here.

Due to accuracy limitations of floating point, there are probably limitations on the ranges we can accept. Do the absolute and relative error thresholds give us enough control here? Do we need to give users any config levers to pull, if we're changing this?

[K-dash]

Thank you for your feedback. Indeed, I agree that we should first clarify what multiple_of should do.

If the result of the multiple_of validation should be the same as Python's value % multiple_of == 0, then the modification I made is inappropriate. Naturally, this is because it doesn't match the results of Python's value % multiple_of == 0, which doesn't allow for rounding errors.

def is_multiple_of(value, multiple_of):
    return value % multiple_of == 0
multiple_of = 0.01
test_values = range(-10, 10)
print(f"Testing for multiple_of = {multiple_of}")
print("-" * 45)
print("value | is_multiple_of | value % multiple_of")
print("-" * 45)
for value in test_values:
    result = is_multiple_of(value, multiple_of)
    remainder = value % multiple_of
    print(f"{value:5} | {str(result):14} | {remainder}")

❯ python test.py                                                                                                                                     
Testing for multiple_of = 0.01
---------------------------------------------
value | is_multiple_of | value % multiple_of
---------------------------------------------
  -10 | False          | 2.0816681711721685e-16
   -9 | False          | 1.8735013540549517e-16
   -8 | False          | 1.6653345369377348e-16
   -7 | False          | 1.457167719820518e-16
   -6 | False          | 1.249000902703301e-16
   -5 | False          | 1.0408340855860843e-16
   -4 | False          | 8.326672684688674e-17
   -3 | False          | 6.245004513516506e-17
   -2 | False          | 4.163336342344337e-17
   -1 | False          | 2.0816681711721685e-17
    0 | True           | 0.0
    1 | False          | 0.00999999999999998
    2 | False          | 0.009999999999999959
    3 | False          | 0.009999999999999938
    4 | False          | 0.009999999999999917
    5 | False          | 0.009999999999999896
    6 | False          | 0.009999999999999875
    7 | False          | 0.009999999999999854
    8 | False          | 0.009999999999999834
    9 | False          | 0.009999999999999813

However, in the existing specification, let threshold = float.abs() / 1e9; is implemented as a rounding tolerance, so I thought there might have been some intention, such as value % multiple_of == 0 alone being too strict.

Due to accuracy limitations of floating point, there are probably limitations on the ranges we can accept. Do the absolute and relative error thresholds give us enough control here? Do we need to give users any config levers to pull, if we're changing this?

Indeed, due to the precision limitations of floating-point numbers, there are restrictions for extremely large or small values. However, we believe the threshold settings we've incorporated can cover most common cases. (If necessary, we can implement more extensive test cases.)
Also, depending on user requirements, it might be worth considering adding an option for users to set the threshold themselves if they need to strictly control extreme edge cases.

In any case, I cannot decide on my own what result the multiple_of validation should return, so I would appreciate your judgment on this matter.

[K-dash]

@davidhewitt @sydney-runkle
I was wondering if you've had a chance to review this yet?
I would greatly appreciate your feedback when you have the opportunity.

[sydney-runkle]

@K-dash, absolutely, will take a close look this afternoon. Thanks so much for your work here!

[sydney-runkle]

Ah, after reviewing again, I do think we need @davidhewitt re:

In any case, I cannot decide on my own what result the multiple_of validation should return, so I would appreciate your judgment on this matter.

Your tests look good to me, but let's defer to DH regarding this logical choice. Will follow up with you tomorrow 👍

[K-dash]

Thank you for checking! I'll be looking forward to response.

[sydney-runkle]

Sure thing. DH is out today, but should be back on Monday and able to respond 👍

[davidhewitt]

This is high on my list of priorities and I've been doing some thinking on it, but I also have some fairly pressing stuff to complete this week. Will do my best to give a full writeup of my thoughts ASAP.

[davidhewitt]

So previously, the algorithm could be described as

1. calculate remainder of division (NB not great for negative numbers)
2. require absolute remainder to be smaller than 1e-9
3. require absolute difference of remainder and multiple_of to also be smaller than 1e-9

The proposal here seems to be to:

• move away from that absolute difference of 1e-9 to something which scales better with multiple_of.
• fix the bug with negative numbers.

Both objectives make sense to me.

I think the thing I'm least clear on is the threshold being scaled back all the way to EPSILON * 100.0. This feels like it is way stricter than the previous implementation?

Should we instead consider making a multiple_of_tolerance parameter, which defaults to the original 1e-9?

[davidhewitt]

This can be summarised as: if multiple_of > 1, then threshold will be increased by the same corresponding factor.

[davidhewitt]

So

• diff > threshold basically is asking for the absolute error to be greater than epsilon (with a factor of 100), possibly scaled up by multiple_of.
• relative_error > epsilon_threshold is basically asking if diff / float is greater than epsilon.

[davidhewitt]

It feels to me like given we evaluate threshold in units of multiple_of, we might want to do similarly here to end up with same scale of multiple_of (by dividing through by rounded)?

Suggested change

	// Calculate the difference between the rounded value and the original value
	let diff = (float - rounded * multiple_of).abs();
	// Calculate how close the rounded division is to multiple_of
	let diff = (float / rounded - multiple_of).abs();

[davidhewitt]

(and sorry for the very long delay, btw)

[sydney-runkle]

@K-dash,

Hope you're well! Any interest in continuing here? Thanks for your work thus far!

[K-dash]

@sydney-runkle
I sincerely apologize for not responding to the feedback earlier (I completely missed the comments...).

While I'm still interested in completing these changes, I currently have other priority tasks that are preventing me from working on this PR. I expect to be able to resume work on this in about a month.

However, if there's any urgency to get these changes released, please feel free to have someone else take over the PR in its current state. I completely understand if that would be a better path forward.

Thank you for your patience and understanding.

[sydney-runkle]

@K-dash,

Sounds great! Good luck with your other work. We're looking forward to further collaborating :)