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

New function to visualise the LexRank graph #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wenyi-tay
Copy link

Hi!

I have been playing around with the lexRankr package and came up with a simple function to plot the graph as I felt it helped me understand LexRank better. Just wondering if this is something valuable to the community. For your consideration =) Thanks!

Cheers, Wenyi

P.S. I am really new to github so I am not sure if I am doing things right. I'll be happy to learn and correct the codes.

@AdamSpannbauer
Copy link
Owner

Hi, @wenyi-tay thanks for the PR! I do think that visualizing the graph would be an interesting addition.

There are some changes that we'll need to make to your file before merging.

  • Your script loads dplyr and tidyr (it appears to work without them). I would like to not add any additional dependencies outside of the ones already listed in DESCRIPTION. These library statements are what caused the tests to fail on your PR (travis results & appveyor results)
  • You've covered just about all the bases in your documentation but we need to convert the documentation to roxygen2 formatting so that it can automatically generate the contents of the man (which in turn generates the documentation when you run ?function_name in an R session). You can see examples in other functions or this tutorial.
  • It's great that you wrote some tests at the end of the script, but we'll need to move tests to the tests/testthat dir to be properly run automatically.

These are the major things that we'll need to do, and we might notice additional improvements in the process.

Are you comfortable with attempting these changes on your own? If not I'd be more than happy for us to work through them together as a learning experience for you. I apologize if this is coming off in a bad way, but I'm assuming you are newer to package development.

Please let me know what you'd like to do moving forward! Thanks again for the PR.

@wenyi-tay
Copy link
Author

Hi! Thanks for the tips! This is my first time trying out to contribute to a package so I will need a lot of guidance. Do let me fix the things you have suggested first and I'll be happy to take any feedback along the way. Thanks! Cheers, Wenyi

@AdamSpannbauer
Copy link
Owner

Hi @wenyi-tay, just checking in. No rush to respond as I'm sure you have other responsibilities, but just wanted to see if you were running into any questions I could help with.

@wenyi-tay
Copy link
Author

Hi @AdamSpannbauer, Thanks for checking in. I've been playing around with the visualisation and I found that it doesn't work very well as the graph gets bigger (my test case was 300+ nodes). I'm thinking of how to make this work so I have not looked into the edits. I'll look into this next weekend and keep you posted. Thanks again! Cheers, Wenyi

@AdamSpannbauer
Copy link
Owner

Making some notes here to investigate later:

  • Visualize using visNetwork as an optional dependency listed under Suggests in DESCRIPTION? (see here for more on Suggests)
    • visNetwork produces interactive visualizations that could leverage hover to display full sentences without cluttering the graph (possibly also show hover information on edges that displays similiarity metric and/or common tokens)
    • An additional step for this function would be to check the presence of the visNetwork on user's machine with require given it's an optional dependency
  • Make visualization more of an investigative tool for a single sentence? (not sure the challenges involved in this, but could be a way to limit size of graph in a different way)

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.

2 participants