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

fix: correct parsing scientific e notation as float #17

Merged
merged 2 commits into from
Mar 31, 2023

Conversation

xin-w8023
Copy link
Contributor

This MR should address #12 by simplify change Loader from yaml to rumel.yaml.

@xin-w8023
Copy link
Contributor Author

For example:

yaml_str = '''
x: 1e-4
y: !ref <x> * 0.1
'''
loaded_yaml = load_hyperpyyaml(yaml_str)
print(loaded_yaml)
print(type(loaded_yaml["x"]), type(loaded_yaml["y"]))

with yaml.Loader, the output will be

{'x': '1e-4', 'y': '1e-05'}
<class 'str'> <class 'str'>

with rumel.yaml.Loader, the output will be

{'x': 0.0001, 'y': 1e-05}
<class 'float'> <class 'float'>

@xin-w8023
Copy link
Contributor Author

@pplantinga hi, could you make a quick review for this MR?

@xin-w8023 xin-w8023 changed the title fix: fix scientific e notation as string fix: fix failed to parse scientific e notation as flaot but string Mar 28, 2023
@xin-w8023 xin-w8023 changed the title fix: fix failed to parse scientific e notation as flaot but string fix: correct parsing scientific e notation as float Mar 28, 2023
@pplantinga
Copy link
Collaborator

This is a known issue with pyyaml yaml/pyyaml#173 implementing the 1.1 spec instead of 1.2. To read these as floats, one could simply use 1.e-4 instead of 1e-4.

While it would be nice to have this, the approach outlined here fails the tests, and looks to me like it would take some considerable effort to get the tests to pass again, since changing the engine is a significant change. If you're willing to put that work in, more power to you, but this is not a pressing enough issue for us to solve it on our side.

@xin-w8023
Copy link
Contributor Author

xin-w8023 commented Mar 29, 2023

This is a known issue with pyyaml yaml/pyyaml#173 implementing the 1.1 spec instead of 1.2. To read these as floats, one could simply use 1.e-4 instead of 1e-4.

While it would be nice to have this, the approach outlined here fails the tests, and looks to me like it would take some considerable effort to get the tests to pass again, since changing the engine is a significant change. If you're willing to put that work in, more power to you, but this is not a pressing enough issue for us to solve it on our side.

Hi, it seems like the Loader didn't add the constructor. I added and it passed all tests offline. Could you approve again to see if it could pass all the tests on CI?

@pplantinga
Copy link
Collaborator

This is an interesting solution, and I'm a little surprised that it works, but in the end it would be nice to solve #8, #12. I tried it with a working recipe, and it seems to work fine. Any objections to this change from @Gastron @TParcollet etc.?

@Gastron
Copy link
Contributor

Gastron commented Mar 30, 2023

Oh interesting that this could solve our issue. I am a little scared because this gets used so much. However, that also means that we should quite quickly find if this runs into issues in some recipes. I am ok with merging, let's just be ready to rollback in case some big issue soon comes up.

@pplantinga pplantinga merged commit f8e881d into speechbrain:main Mar 31, 2023
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.

hyperpyyaml sometimes loads scientific e notation as string Float gets parsed as string
3 participants