Skip to content

fix: correct parsing scientific e notation as float#17

Merged
pplantinga merged 2 commits into
speechbrain:mainfrom
xin-w8023:main
Mar 31, 2023
Merged

fix: correct parsing scientific e notation as float#17
pplantinga merged 2 commits into
speechbrain:mainfrom
xin-w8023:main

Conversation

@xin-w8023

Copy link
Copy Markdown
Contributor

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

@xin-w8023

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

xin-w8023 commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

Gastron commented Mar 30, 2023

Copy link
Copy Markdown
Contributor

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