Repository navigation
add global float_precision config - #1470
Conversation
Vincent-Maladiere
left a comment
Vincent-Maladiere
left a comment
There was a problem hiding this comment.
Hey @georgescutelnicu, the PR is already in good shape! Some feedback
| local_config["enable_subsampling"] = enable_subsampling | ||
|
|
||
| if float_precision is not None: | ||
| if not isinstance(float_precision, numbers.Integral) or float_precision <= 0: |
There was a problem hiding this comment.
A bit of a nitpick: should we allow float_precision = 0? This would display integer values like 1.24 -> 1
There was a problem hiding this comment.
isn't setting it to 0 giving the same output as setting it to 1 since the formatting uses significant digits rather than fixed decimal places?
From python docs (string — Common string operations): 'g' General format. For a given precision p >= 1, this rounds the number to p significant digits and then formats the result in either fixed-point format or in scientific notation, depending on its magnitude. A precision of 0 is treated as equivalent to a precision of 1.
There was a problem hiding this comment.
Yes, my understanding was slightly off, but the question remains. If 0 is equivalent to 1, should we keep it?
There was a problem hiding this comment.
i thought it might confuse users into thinking 0 behaved differently, but since its treated the same as 1, we could keep it allowing for flexibility i guess, since it doesnt seem to cause any harm
There was a problem hiding this comment.
Ok, I don't have a strong preference
Vincent-Maladiere
left a comment
Vincent-Maladiere
left a comment
There was a problem hiding this comment.
LGTM! Thanks @georgescutelnicu
rcap107
left a comment
rcap107
left a comment
There was a problem hiding this comment.
Looks good! Thanks a lot @georgescutelnicu
4 participants
Related to #1410
This is a follow up to #1463 pr. The earlier approach handled
float_precisiondirectly inTableReport. Im opening this new pr to keep the history clean and focused on the new approach.In the test i used the target
y = fetch_employee_salaries().yinstead ofXbecause we needed a float column to checkfloat_precisionagainst.