CDK-R / cdkr

Integrating R and the CDK
https://cdk-r.github.io/cdkr/
42 stars 27 forks source link

request rcdklibs 1.5.12 release #31

Closed zachcp closed 8 years ago

zachcp commented 8 years ago

hi @rajarshi,

thanks for your excellent package. I have been writing some utilities that depend on rcdk/rcdklibs and I would like to use the most recent features of 1.5.12 including the newer smiles parsing and possibly image depiction. Therefore, I am hoping that you can cut another CRAN release of rcdk/rcdklibs. I'd be happy to help in any way.

(FYI: just put together a chemdoodle widget for drawing molecules in html and using CDK as the backend for parsing them https://github.com/zachcp/chemdoodle)

zach cp

zachcp commented 8 years ago

hoping this can be done after inclusion of the cdk-depict Jar added in #33. Can I assist in any way?

rajarshi commented 8 years ago

No issues with pushing out a new rcdklibs version - but this will also probably need to push out the corresponding rcdk version as well. I'm a bit swamped with work this week, so if you don't mind checkout out the code and making sure the package builds with no errors, you could add yourself as a maintainer to rcdk and rcdklibs and push them to CRAN. Otherwise I'll do it next weekend

zachcp commented 8 years ago

thanks @rajarshi . I'm happy to try to build and submit.

I've run into the following error on building:

Error: processing vignette 'rcdk.Rnw' failed with diagnostics:
 chunk 25
Error in FUN(X[[i]], ...) : unused argument (check = FALSE)
Execution halted

The offending chunk (below) works when manually entered or when built in RStudio's editor.

<<>>=
aDesc <- eval.desc(mol, dn[1])
allDescs <- eval.desc(mol, dn)
@ 

Using commenting it seems allDescs <- eval.desc(mol, dn) is the issue.

Any ideas whats going on? Theres no debugging in knitr when building vignettes and I can't reproduce the bug otehrwise.

zach cp

rajarshi commented 8 years ago

I'm guessing this might be an issue in an rJava call. I can dig in to this more towards the end of the week

On Sun, Mar 20, 2016 at 9:55 PM, zachcp notifications@github.com wrote:

thanks @rajarshi https://github.com/rajarshi . I'm happy to try to build and submit.

I've run into the following error on building:

Error: processing vignette 'rcdk.Rnw' failed with diagnostics: chunk 25 Error in FUN(X[[i]], ...) : unused argument (check = FALSE) Execution halted

The offending chunk (below) works when manually entered or when built in RStudio's editor.

<<>>=aDesc <- eval.desc(mol, dn[1])allDescs <- eval.desc(mol, dn)@

Using commenting it seems allDescs <- eval.desc(mol, dn) is the issue.

Any ideas whats going on? Theres no debugging in knitr when building vignettes https://groups.google.com/forum/#!topic/knitr/_KQRjcEMAQs and I can't reproduce the bug otehrwise.

zach cp

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/rajarshi/cdkr/issues/31#issuecomment-199083944

Rajarshi Guha | http://blog.rguha.net NIH Center for Advancing Translational Science

zachcp commented 8 years ago

hi @rajarshi, is there any chance you've had any time to look at this?

rajarshi commented 8 years ago

Oops, completely lost track of this. Let me look in to this today or tomorrow

rajarshi commented 8 years ago

I just pushed a few fixes - this fixes he vignette issue on my side. Can you give it a test and see if also works for you?

zachcp commented 8 years ago

thanks.

I can build the package now but get a few errors during the build step. I fixed a few of them in PR #34 but there are still two notes:

Found the following apparent S3 methods exported but not registered:
  hasNext.iload.molecules
See section ‘Registering S3 methods’ in the ‘Writing R Extensions’
manual.

* checking package dependencies ... NOTE
  No repository set, so cyclic dependency check skipped

zach cp

zachcp commented 8 years ago

ran into an issue with travis. if its not excluded in .Rbuildignore it gets noticed by R CMD CHECK. But when you put it in .Rbuildignore (like ggplot2 or covr) then travis won't build. I think the issue is that we want ot build a subdirectory. So I moved .travis to the root directory and use subcommand and travis runs but fails on import of the rcdklibs version dependency.

In any event, even though my PR is failing travis I expect it to pass if rcdklibs is uploaded.

zcp

rajarshi commented 8 years ago

for now I switched off travis builds - once a new release gets pushed out I'll poke around and see if I can get it working. The bundling of multiple packages in to a single repo is probably not the best approach

On Wed, Apr 27, 2016 at 10:11 PM, zachcp notifications@github.com wrote:

ran into an issue with travis. if its not excluded in .Rbuildignore it gets noticed by R CMD CHECK. But when you put it in .Rbuildignore (like ggplot2 or covr) then travis won't build. I think the issue is that we want ot build a subdirectory. So I moved .travis to the root directory and use subcommand and travis runs but fails on import of the rcdklibs version dependency.

In any event, even though my PR is failing travis I expect it to pass if rcdklibs is uploaded.

zcp

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/rajarshi/cdkr/issues/31#issuecomment-215287956

Rajarshi Guha | http://blog.rguha.net NIH Center for Advancing Translational Science

zachcp commented 8 years ago

looks like it can be done: https://lord.io/blog/2014/travis-multiple-subdirs/ the main issue is if cdkr requires acdkrlib that is not yet on CRAN.

thanks for working on this.

rajarshi commented 8 years ago

re the S3 error above, what version of R are you using? I'm not seeing this error with R CMD CHECK

zachcp commented 8 years ago

R: 3.2.3 on OSX. However, I just did a fresh git clone from your repo and everything built with no errors.

Thanks!

rajarshi commented 8 years ago

great - will you push out rcdk and rcdklibs to CRAN? If not, I'll push them this weekend.

(BTW, do you know how to get a new installation of R and rJava to recognize JAVA_HOME? For some reason .jcall("java/lang/System","S","getProperty","java.home") is giving me my 1.6 home and not the one set via JAVA_HOME)

zachcp commented 8 years ago

I can push them to CRAN.

As for Java it might be worth setting a warning somewhere in rcdk because almost certainly people will encounter the same error. Heres what I did on OSX 10.11 to get rJAVA to use the latest version of Java:

#link, reconfigure, rebuild
sudo ln -f -s $(/usr/libexec/java_home)/jre/lib/server/libjvm.dylib /usr/lib
sudo R CMD javareconf
#from R
install.packages('rJava', type="source")

I am not sure if this is the recommended/best way to register rJava, however, its the only way I could find on StackOverflow that worked.

rajarshi commented 8 years ago

Thanks (btw, since you're pushing, you could add yourself as maintainer if you'd like).

re the registering rJava - those instructions broke my installation :)

zachcp commented 8 years ago

new version - 1.5.13 - released on CRAN.

It might be nice to have an alternative way to distribute JAR files as this was the major issue I had to argue for acceptance - and rcdklibs clocks in at ~25Mb where the suggested limit is 5mb. Maybe there is a maven plugin for rJava that can manaage Java dependencies?