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

Fixed stripping of . from extracted numbers #136

Closed
wants to merge 4 commits into from
Closed

Fixed stripping of . from extracted numbers #136

wants to merge 4 commits into from

Conversation

stanstrup
Copy link
Collaborator

Right now numbers are not extracted corrected.
For examples in pp_query.

extr_num("Melting Pt : -44.6 deg C") strips the "." and gives -446.

My fix keeps "."'s. It will fail if there is a dot in another place though.
extr_num("Melting Pt : -44.6 deg C.") gives NA.

Alternatively you could use readr::parse_number that also handles that.

Right now numbers are not extracted corrected.
For examples in pp_query.

`extr_num("Melting Pt : -44.6 deg C")` strips the "." and gives -446.

My fix keeps "."'s. It will fail if there is a dot in another place though.
`extr_num("Melting Pt : -44.6 deg C.")` gives NA.

Alternatively you could use `readr::parse_number` that also handles that.
@stanstrup
Copy link
Collaborator Author

Hmm. This strips the - instead.

@stanstrup
Copy link
Collaborator Author

OK fixed that.

@codecov-io
Copy link

codecov-io commented Nov 23, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@46cb217). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master   #136   +/-   ##
=======================================
  Coverage          ?     0%           
=======================================
  Files             ?     17           
  Lines             ?   1622           
  Branches          ?      0           
=======================================
  Hits              ?      0           
  Misses            ?   1622           
  Partials          ?      0
Impacted Files Coverage Δ
R/utils.R 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46cb217...4b897f1. Read the comment docs.

Evidently CS is sometimes not escaping properly special characters.
Here is a fix for the problem I found. I could not find a general way to
fix this. I guess if there was one parsers would do it.
Some chemspider records are missing epiSuite data. For those the parsing
will fail.
@eduardszoecs
Copy link
Member

Cherry-picked d2201f8 , added tests & merged into master. Closes #136 .

Can you separate PRs regarding issues (easier for me to check & merge). Will now look at #138 and #139.

Thank you for your welcome contributions!

eduardszoecs added a commit that referenced this pull request Nov 25, 2017
@eduardszoecs
Copy link
Member

Cherry-picked all suggestions.

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.

None yet

3 participants