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

bugfix for is.smiles #140

Closed
wants to merge 1 commit into from
Closed

bugfix for is.smiles #140

wants to merge 1 commit into from

Conversation

allaway
Copy link

@allaway allaway commented Jan 11, 2018

Hello - I recently ran into an issue with this function using the current CRAN version of webchem, but I believe this issue still exists in the master branch here. Apologies if I am supposed to file a pull request against a different branch than master.

is.smiles previously returned TRUE for any string regardless of validity. I suspect this was due to changes in rcdk::parse.smiles.

This change should correctly return TRUE for valid SMILES, and FALSE for un-parseable SMILES.

is.smiles previously returned TRUE for invalid SMILES. Suspect this was due to changes in rcdk::parse.smiles. 
This update should correctly return TRUE for valid SMILES, and FALSE for un-parseable SMILES.
@codecov-io
Copy link

codecov-io commented Jan 11, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##             master   #140   +/-   ##
=======================================
  Coverage          ?     0%           
=======================================
  Files             ?     17           
  Lines             ?   1631           
  Branches          ?      0           
=======================================
  Hits              ?      0           
  Misses            ?   1631           
  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 10a8858...bffa4be. Read the comment docs.

@eduardszoecs
Copy link
Member

Thanks for your contribution! I'll check and test ASAP (might take few days).
In fact, i had to turn off the tests for is. smiles() , as java is a bit b****y 😞

Could also use rdkit the test if it parses (could be implemented via rrdkit, or (better) via reticulate.

@allaway
Copy link
Author

allaway commented Jan 12, 2018

Eliminating the rJava dependency (via removing dependency on rcdk) would not be a bad thing! :)

eduardszoecs added a commit that referenced this pull request Jan 15, 2018
@eduardszoecs
Copy link
Member

Thanks for your contribution!
Incorporated you changes in master and fixed tests and continuous integration (rcdk installation failed).

@allaway allaway deleted the patch-1 branch February 6, 2019 04:27
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