-
Notifications
You must be signed in to change notification settings - Fork 800
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
better handle float value comparison #407
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vandyxiaowei,
Thanks for the PR!
However, we cannot merge it because the line value = float(value)
will convert any value to a float
when the parameter is expecting a float value, even if the original model-provided value is of type int
. This compromises the purpose of type-checking in the AST evaluation process.
The reason why we think strict typing is necessary can be found in a previous issue reply. The TLDR is that, without looking at the source code, it’s not 100% safe to say that the Python auto-conversion between float and int would always work as expected. Thus in the AST test category, we need to be precise with typing. There are certainly many scenarios where providing a float value for an integer parameter would work perfectly fine, and that’s exactly the purpose of the executable test category, where we are concerned only with the execution result (as opposed to how the result is obtained).
PS, you might noticed that in line 397, we manually convert the value to list
when the expected type is tuple
, eg value = list(value)
—- This is technically a workaround rather than best practice (and it does compromise the purpose of type-checking as well). The conversion is necessary there because any tuple
in the possible answer would become a list after being processed through json.dump()
and json.load()
. I will have a more robust solution to address this issue :)
Hi @HuanzhiMao, I just read again your response to the attached issue (which I opened) and I still disagree. I'm commenting here because the attached issue is already closed. In your response you demonstrate why it's not okay to send a |
Hey @danieljannai21, Sorry that issue was closed. I don't have admin access to re-open it, RIP:/ But you can always open a new follow-up issue :) Regarding your first concern on why the Python auto-conversion from The TLDR is that you cannot represent every
Now, suppose I have the following function (you probably have seen such interest-rate-calculation-function in the BFCL dataset quite a lot haha):
Since we are talking about accuracy here, let's use a high-precision version of the above function.
The result is below. Sending an
Now regarding your second concern that the OpenAPI standard allows float in
Let me know if you have more questions! Happy to discuss this further with you :) |
Hey @HuanzhiMao, Thanks for taking the time to answer my concerns. The problem in your example is that the number was inaccurately the moment it entered a |
No description provided.