wiseio / paratext

A library for reading text files over multiple cores.
Apache License 2.0
1.06k stars 103 forks source link

Type Detection #30

Closed gridseth closed 7 years ago

gridseth commented 8 years ago

Summary:

When parsing files I ran into problems with how paratext parses some of the elements in a file. I believe these may be related to the process_token() function in colbased_worker.hpp that tries to detect whether a token is an integer, float, exponential/scientific number or otherwise.

I have written code to make changes to this function, but was not able to create a pull request. Please let me know if there is a way I can create a pull request or provide the code change suggestions.

Examples:

It detects a string such as A.1 as a number and I get 0.000000 It detects a string such as 3ABC as a number and I get 3.000000

Details:

Let A.1 be the input. When reading the token and checking if the token is an integer (line 270) it checks if token_[i] is a digit. If not, we move on to see if we are dealing with a float instead. However we advance the index, i, regardless of whether the integer check passes or fails. Therefore, when we get to the float check on line 279 we are looking at the . character instead of A. Then the check for a float passes since we see . and only digits after it. Finally the result is 0.000000 since A.1 gets converted to a float before it is passed to process_float.

Numbers like 12.345 are not picked up as floats (because the integer check fails on . and we check for float on the next character, here 3), but instead as exponentials. They pass as exponentials not because they pass the exponential check, but because exp_possible is set as true at the beginning (on line 272 after the integer check passes on line 270) and does not become false. Both exponentials and floats are passed to process_float in the end. For the same reason 3ABC is detected as an exponential instead of a string and we get 3.000000. (Numbers like .123 are not detected as floats or exponentials because exp_possible is set as false after the integer check .)

Note:

When making updates to the process_token() function I did some simplifications, but did not change the behaviour of the function other than for the issues found.

In the code change suggestion I have let a number on the form 14e-3 be a valid exponential (compared to 14.0e-3). This can be changed if not desired.

catchmrbharath commented 8 years ago

@gridseth Do you need help in sending a pull request?

gridseth commented 8 years ago

@catchmrbharath Thanks. I am getting some help now, so I'll be able to submit soon.

catchmrbharath commented 8 years ago
gridseth commented 8 years ago

I have added the pull request, thanks for the help! https://github.com/wiseio/paratext/pull/31

deads commented 8 years ago

Thank you for the bug report and the proposed fix. I will look it over.

deads commented 7 years ago

This issue should be resolved given the latest merged PRs. If not, please reopen. Thank you for your input and contributions.