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

String to float conversion edge case #838

Closed
rsokl opened this issue Dec 6, 2021 · 5 comments
Closed

String to float conversion edge case #838

rsokl opened this issue Dec 6, 2021 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@rsokl
Copy link

rsokl commented Dec 6, 2021

Hello! Hypothesis found an odd test case during one of hydra-zen's nightly jobs.

The yaml field-value '0_e0' gets converted to a float:

>>> instantiate(OmegaConf.create('x: 0_e0'))
{'x': 0.0}

however this string is not actually a valid representation of 0.0:

>>> float('0_e0')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-16-12374ef4f47c> in <module>
----> 1 float('0_e0')

I do not think that the underscore can precede e directly.

@rsokl rsokl added the bug Something isn't working label Dec 6, 2021
@rsokl
Copy link
Author

rsokl commented Dec 6, 2021

Actually, this might be more appropriate for Hydra, since it involves instantiation. If so, I will be happy to move it.

@Jasha10
Copy link
Collaborator

Jasha10 commented Dec 6, 2021

I think that the culprit is this regex in OmegaConf's custom yaml loader :)

@rsokl
Copy link
Author

rsokl commented Dec 6, 2021

Yup! That sure looks like it!

@pixelb pixelb self-assigned this Aug 4, 2022
@pixelb
Copy link
Collaborator

pixelb commented Aug 9, 2022

tl;dr we should probably restrict underscore handling to less cases.
At least we should disallow trailing underscores (as is the case in this bug).

That would match most underscore allowing languages including python.
See https://peps.python.org/pep-0515/

I see yaml 1.2 doesn't even specify _ as allowed in numbers, as the 1.1 spec was too loosely defined wrt _
See: nodeca/js-yaml#627

I see our existing RE probably originates in ruamel.
See https://stackoverflow.com/q/30458977/4421 and commit a50bdee

It's interesting that yaml being a superset of json, diverges from json here ?

pixelb added a commit to pixelb/omegaconf that referenced this issue Aug 10, 2022
Disallow numbers with trailing underscore from being converted to float.

The specific change here is to treat 0_e0 as a string.

Fixes issue omry#838
@pixelb
Copy link
Collaborator

pixelb commented Aug 10, 2022

I also see some inconsistencies between the antlr (interpolation) and basic loader.
pr #995 does not address that, but at least documents it.

pixelb added a commit that referenced this issue Aug 10, 2022
Disallow numbers with trailing underscore from being converted to float.

The specific change here is to treat 0_e0 as a string.

Fixes issue #838
pixelb added a commit to pixelb/omegaconf that referenced this issue Aug 10, 2022
Disallow numbers with trailing underscore from being converted to float.

The specific change here is to treat 0_e0 as a string.

Fixes issue omry#838
pixelb added a commit that referenced this issue Aug 10, 2022
Disallow numbers with trailing underscore from being converted to float.

The specific change here is to treat 0_e0 as a string.

Fixes issue #838
@pixelb pixelb closed this as completed Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants