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

[stdlib] Add method atof() to String #2649

Closed
wants to merge 20 commits into from

Conversation

fknfilewalker
Copy link

@fknfilewalker fknfilewalker commented May 14, 2024

This PR adds a function that can convert a String to a Float64. Right now it is implemented just for Float64 but maybe we should add other precisions?

This supports the following notations:

"-1236.233"
"2.25"
"2."
"1.7E+3"
# as well as the f/F postfix notation
"-1236.233f"
"2.25F"
"2.f"
"1.7E+3F"

@fknfilewalker fknfilewalker requested a review from a team as a code owner May 14, 2024 11:16
@fknfilewalker fknfilewalker changed the title An atof implementation (String to Float) [stdlib] Add method atof() to String May 14, 2024
@fknfilewalker fknfilewalker changed the title [stdlib] Add method atof() to String [stdlib] Add method atof() to String May 14, 2024
@Moosems
Copy link

Moosems commented May 14, 2024

I think this also needs tests

@fknfilewalker
Copy link
Author

fknfilewalker commented May 14, 2024

What do I have to do to make the 'Standard Library tests and examples' test pass?

@artemiogr97
Copy link
Contributor

What do I have to do to make the 'Standard Library tests and examples' test pass?

@fknfilewalker seems like some files are not well formatted, try running mojo format . and commit your changes

@artemiogr97
Copy link
Contributor

artemiogr97 commented May 15, 2024

@fknfilewalker there is still the file 'test_string' that needs to be formated, that's why the test are failing

Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
@Moosems
Copy link

Moosems commented May 15, 2024

@JoeLoser Would you be willing to review this or get someone from the proper team? Hope you're having a good day!

fknfilewalker and others added 6 commits May 15, 2024 17:40
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Copy link
Contributor

@laszlokindrat laszlokindrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch, this is gonna be awesome! I have a few suggestions and quibbles, but it's generally looking nice!

stdlib/src/builtin/string.mojo Outdated Show resolved Hide resolved
stdlib/src/builtin/string.mojo Outdated Show resolved Hide resolved
stdlib/src/builtin/string.mojo Outdated Show resolved Hide resolved
stdlib/src/builtin/string.mojo Outdated Show resolved Hide resolved
Comment on lines +492 to +496
var shift: Int = 10 ** abs(exponent)
if exponent > 0:
result *= shift
if exponent < 0:
result /= shift
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since result is a Float64 this will implicitly convert shift anyway. Does this work?

Suggested change
var shift: Int = 10 ** abs(exponent)
if exponent > 0:
result *= shift
if exponent < 0:
result /= shift
var result *= 10.0 ** exponent

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem here is that mojo rounds the floats differently, and doing a division for negative exponents is closer to how mojo does it.
e.g., -0.3
mojo stores it as -0.29999999999999999
while the multiply method (3.0 * 0.1) returns -0.30000000000000004
the division method (3.0 / 10.0) produces -0.29999999999999999

so the result is different depending on whether doing a * 10 or / 0.1 or vice versa

A similar strange situation is the following

print(233.0 / 1000.0) #prints 0.23299999999999998
var x: Float64 = 233.0
var div: Float64 = 1000.0
print(x/div) #prints 0.23300000000000001

If this is a compiler issue and we can fix that then this suggestion would be definitely the better option

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this makes sense actually. Can you please ensure we have at least one test case that hits this? Please leave a comment both here and at that test case to explain this. Thank you!

stdlib/src/builtin/string.mojo Outdated Show resolved Hide resolved
stdlib/test/builtin/test_string.mojo Show resolved Hide resolved
fknfilewalker and others added 4 commits May 16, 2024 19:11
Co-authored-by: Laszlo Kindrat <laszlokindrat@gmail.com>
Signed-off-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Co-authored-by: Laszlo Kindrat <laszlokindrat@gmail.com>
Signed-off-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
@laszlokindrat
Copy link
Contributor

Also, please add a changelog entry!

Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
@fknfilewalker fknfilewalker requested a review from a team as a code owner May 16, 2024 19:38
fknfilewalker and others added 3 commits May 16, 2024 23:53
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Signed-off-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
modularbot added a commit that referenced this pull request May 17, 2024
[External] [stdlib] Add method `strip()` to `StringRef`

This PR adds a `strip` method to `StringRef`. This PR is helpful for
#2649.

Where can I find the test cases for StringRef?

ORIGINAL_AUTHOR=Lukas Lipp
<15105596+fknfilewalker@users.noreply.github.com>
PUBLIC_PR_LINK=#2683

Co-authored-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Closes #2683
MODULAR_ORIG_COMMIT_REV_ID: 5ac8b1f3b45c75a964c5d9368e3871e7fc617a88
fknfilewalker and others added 3 commits May 17, 2024 10:12
Signed-off-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
@laszlokindrat laszlokindrat added the imported-internally Signals that a given pull request has been imported internally. label May 17, 2024
Copy link
Contributor

@laszlokindrat laszlokindrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great patch, thank you so much!

@laszlokindrat
Copy link
Contributor

@JoeLoser Would it make sense to make this functionality parametric on a floating point type? I.e. something like

fn atof[type: Dtype = Dtype.float64](str: String) raises -> Scalar[dtype]:
    constrained[type.is_float(), "must be float"]()
    ...

@laszlokindrat
Copy link
Contributor

@fknfilewalker If you are interested, you could also do an overload for StringLiterals that returns arbitrary precision floats:

fn atof(str_literal: StringLiteral) -> FloatLiteral: ...

@JoeLoser
Copy link
Collaborator

@JoeLoser Would it make sense to make this functionality parametric on a floating point type? I.e. something like

fn atof[type: Dtype = Dtype.float64](str: String) raises -> Scalar[dtype]:
    constrained[type.is_float(), "must be float"]()
    ...

Is your goal to make it work for arbitrary-precision floats, or something else?

@laszlokindrat
Copy link
Contributor

@JoeLoser Would it make sense to make this functionality parametric on a floating point type? I.e. something like

fn atof[type: Dtype = Dtype.float64](str: String) raises -> Scalar[dtype]:
    constrained[type.is_float(), "must be float"]()
    ...

Is your goal to make it work for arbitrary-precision floats, or something else?

Making it work for FloatLiteral would be nice, but there might be use cases where it's important not to have to go through Float64 if wanting to go to Float32 or Float16 (because it might actually give different results due to rounding).

@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels May 17, 2024
@modularbot
Copy link
Collaborator

Landed in 8641d49! Thank you for your contribution 🎉

modularbot added a commit that referenced this pull request May 18, 2024
[External] [stdlib] Add method `atof()` to `String`

This PR adds a function that can convert a `String` to a `Float64`.
Right now it is implemented just for Float64 but maybe we should add
other precisions?

This supports the following notations:
```python
"-1236.233"
"2.25"
"2."
"1.7E+3"
# as well as the f/F postfix notation
"-1236.233f"
"2.25F"
"2.f"
"1.7E+3F"
```

ORIGINAL_AUTHOR=Lukas Lipp
<15105596+fknfilewalker@users.noreply.github.com>
PUBLIC_PR_LINK=#2649

Co-authored-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Closes #2649
MODULAR_ORIG_COMMIT_REV_ID: b8d2c4ef38faa639e749957a0c1ba1a9c02a28cf
@modularbot modularbot closed this May 18, 2024
@fknfilewalker fknfilewalker deleted the atof branch May 18, 2024 08:33
@fknfilewalker
Copy link
Author

fknfilewalker commented May 18, 2024

@fknfilewalker If you are interested, you could also do an overload for StringLiterals that returns arbitrary precision floats:

fn atof(str_literal: StringLiteral) -> FloatLiteral: ...

Sure, is there a reason why we have the inner _atof function (same for _atol)?

@fknfilewalker
Copy link
Author

@JoeLoser Would it make sense to make this functionality parametric on a floating point type? I.e. something like

fn atof[type: Dtype = Dtype.float64](str: String) raises -> Scalar[dtype]:
    constrained[type.is_float(), "must be float"]()
    ...

Is your goal to make it work for arbitrary-precision floats, or something else?

Making it work for FloatLiteral would be nice, but there might be use cases where it's important not to have to go through Float64 if wanting to go to Float32 or Float16 (because it might actually give different results due to rounding).

In order for this to work with FloatLiteral we would need to use StringLiteral I guess? How does it work to create a function for runtime values and compile time values?

@fknfilewalker
Copy link
Author

Could atof() accept a Stringable instead of a String? Then everything could be used?

msaelices pushed a commit to msaelices/mojo that referenced this pull request May 18, 2024
[External] [stdlib] Add method `strip()` to `StringRef`

This PR adds a `strip` method to `StringRef`. This PR is helpful for
modularml#2649.

Where can I find the test cases for StringRef?

ORIGINAL_AUTHOR=Lukas Lipp
<15105596+fknfilewalker@users.noreply.github.com>
PUBLIC_PR_LINK=modularml#2683

Co-authored-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Closes modularml#2683
MODULAR_ORIG_COMMIT_REV_ID: 5ac8b1f3b45c75a964c5d9368e3871e7fc617a88
msaelices pushed a commit to msaelices/mojo that referenced this pull request May 18, 2024
[External] [stdlib] Add method `atof()` to `String`

This PR adds a function that can convert a `String` to a `Float64`.
Right now it is implemented just for Float64 but maybe we should add
other precisions?

This supports the following notations:
```python
"-1236.233"
"2.25"
"2."
"1.7E+3"
# as well as the f/F postfix notation
"-1236.233f"
"2.25F"
"2.f"
"1.7E+3F"
```

ORIGINAL_AUTHOR=Lukas Lipp
<15105596+fknfilewalker@users.noreply.github.com>
PUBLIC_PR_LINK=modularml#2649

Co-authored-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Closes modularml#2649
MODULAR_ORIG_COMMIT_REV_ID: b8d2c4ef38faa639e749957a0c1ba1a9c02a28cf
@laszlokindrat
Copy link
Contributor

Sure, is there a reason why we have the inner _atof function (same for _atol)?

I don't think so, maybe the implementation used to be different and this was left over.

In order for this to work with FloatLiteral we would need to use StringLiteral I guess?

Yes.

Could atof() accept a Stringable instead of a String?

Yes, but notice that the overload I suggested returns a FloatLiteral, not a Scalar[...]:

fn atof(str_literal: StringLiteral) -> FloatLiteral: ...

FloatLiteral is arbitrary precision, and they idea here would be that this function can only be invoked at compile time. For this to work, we need to make StringLiteral @nonmaterializable, which is something I'm working on.

martinvuyk pushed a commit to martinvuyk/mojo that referenced this pull request May 24, 2024
[External] [stdlib] Add method `strip()` to `StringRef`

This PR adds a `strip` method to `StringRef`. This PR is helpful for
modularml#2649.

Where can I find the test cases for StringRef?

ORIGINAL_AUTHOR=Lukas Lipp
<15105596+fknfilewalker@users.noreply.github.com>
PUBLIC_PR_LINK=modularml#2683

Co-authored-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Closes modularml#2683
MODULAR_ORIG_COMMIT_REV_ID: 5ac8b1f3b45c75a964c5d9368e3871e7fc617a88
martinvuyk pushed a commit to martinvuyk/mojo that referenced this pull request May 24, 2024
[External] [stdlib] Add method `atof()` to `String`

This PR adds a function that can convert a `String` to a `Float64`.
Right now it is implemented just for Float64 but maybe we should add
other precisions?

This supports the following notations:
```python
"-1236.233"
"2.25"
"2."
"1.7E+3"
"-1236.233f"
"2.25F"
"2.f"
"1.7E+3F"
```

ORIGINAL_AUTHOR=Lukas Lipp
<15105596+fknfilewalker@users.noreply.github.com>
PUBLIC_PR_LINK=modularml#2649

Co-authored-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Closes modularml#2649
MODULAR_ORIG_COMMIT_REV_ID: b8d2c4ef38faa639e749957a0c1ba1a9c02a28cf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants