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

Use sg lightweight instead of igraph #151

Closed
gaborcsardi opened this issue May 17, 2016 · 5 comments
Closed

Use sg lightweight instead of igraph #151

gaborcsardi opened this issue May 17, 2016 · 5 comments

Comments

@gaborcsardi
Copy link
Member

If you are interested, I will provide a PR soonish. Either for switching to https://github.com/mangothecat/simplegraph which is R-only and has no dependencies, or custom code. It is fairly trivial.

@jimhester
Copy link
Member

Sounds good to me, I had initially used igraph as a performance improvement, but it is a heavy dependency for sure.

@gaborcsardi
Copy link
Member Author

Do you have an idea how big the graphs are?

@jimhester
Copy link
Member

Depends on the file you are linting I guess, I was testing on https://github.com/wch/r-source/blob/trunk/src/library/tools/R/QC.R which is ~8300 lines long. Lintr splits the parse data data frames by expression, looks like the largest dataframe is 6652 rows, which I guess is fairly small...

@gaborcsardi
Copy link
Member Author

gaborcsardi commented May 17, 2016

Indeed, my quick and dirty R implementation will not do. For QC.R it increases running time from 70 sec to 223 sec. https://github.com/gaborcsardi/lintr/commit/7121cf8d86b547539f7ebc773910bd05667eccac

I'll try sg else in a minute.

Btw. is this graph component stuff only needed for splitting the file into expressions? Because then maybe I can ~~~start~~~ try sg else.

Btw. 2. it is easy to write this in C as well, is that preferable to igraph? I am not sure myself. Maybe the best would be some micro-package dependency that only does this, but fast (= using C code).

@gaborcsardi
Copy link
Member Author

OK, the bottleneck was a stupid type conversion (did you know that modifyList is super slow? It makes sense if you look at its code...), and now it is actually faster than the current lintr code, it is ~61 sec for QC.R. Hopefully it is also correct. :) Let me run some more tests, and then I'll submit a PR.

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

No branches or pull requests

2 participants