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

suggestion to integrate the R wrapper in the repository #165

Closed
mlampros opened this issue Mar 13, 2018 · 20 comments
Closed

suggestion to integrate the R wrapper in the repository #165

mlampros opened this issue Mar 13, 2018 · 20 comments

Comments

@mlampros
Copy link
Collaborator

This issue is related with a previous one.
A month ago I wrapped rgf_python using the reticulate package in R. It can be installed on Linux, and somehow cumbersome on Macintosh and Windows (on Windows currently it works only from the command prompt).
I opened the issue as suggested by @fukatani

@fukatani
Copy link
Member

I'm sorry for late response.
Though it is not a situation where work can be started immediately, I want to merge in the future.

@mlampros mlampros reopened this Mar 17, 2018
@mlampros
Copy link
Collaborator Author

I'm sorry too,
I thought it's not a good idea because the package doesn't work on Windows as it works on Linux and Macintosh. I reopened the issue, so you can notify me when you think that merging (repository) should take place.

@mlampros
Copy link
Collaborator Author

mlampros commented Jul 26, 2018

Hi again,

@fukatani, @StrikerRUS I upgraded the RGF package so that it corresponds to the most recent version of rgf_python (I updated the installation instructions for all 3 operating systems too). I added also Singularity images (similar to docker images) and I explained in a blog post how these can be installed both in R and Python, in case for instance that users are not able to install locally.

This issue is already 5 months old thus I thought I asked once again if the R package (in its current state) should be included to rgf or it would be better to close the issue. Thanks in any case.

@StrikerRUS
Copy link
Member

StrikerRUS commented Jul 26, 2018

Hi @mlampros !

Sorry, we were busy with repository reorganization (#166) recently which blocked R-package integration.

Thanks for reminding us.

My opinion is that your contribution will be very valuable for the repository and ML/DS-community in generally (I mean, all RGF codebase in one place). Unfortunately, I'm not familiar with R, just basic knowledge. What do you think, @fukatani? Can we start the process of integrating R-package?

I saw you have Travis CI tests, I suppose we could move your .travis.yml file into a bash script and assign for it a separate build job.

@mlampros
Copy link
Collaborator Author

@StrikerRUS I'm unfamiliar with the integration / merging of Github repositories, is there a specific process that I should follow. For instance, do I have to convert the .travis.yml to a bash script in first place?

@StrikerRUS
Copy link
Member

@mlampros To be honest, I'm too :-)

The final goal is to move your code to RGF-team/rgf/R-package.

I see two alternatives:

  • you create a PR with the content of your repo;
  • you transfer the ownership of your repo to the RGF-team and then we together create a PR.

I don't know when it's better to make changes, e.g. convert .travis.yml file into bash script, and I suppose it's not the only change that should be done...

@mlampros
Copy link
Collaborator Author

@StrikerRUS, ok, I'll create a PR tomorrow, but first things first I think I'll have to wait for fukatani's response.

@fukatani
Copy link
Member

fukatani commented Jul 27, 2018

@mlampros Sorry for very late response and thank you for your suggestion!

Integration brings benefits to users. I want to integrate repository.

But we have some challenge.
We don't have basic knowledge about R, so you have to join and continue maintainance as RGF team member to integrate repository.

So I want to ask, could you collaborate with us?

As @StrikerRUS referred, test automation is important. Does it work well if it can do it?
Deployment seems to be only an update of github.

@fukatani
Copy link
Member

fukatani commented Jul 27, 2018

I think integration bring us these three benefits.

  • Stable deployment
    If rgf_python API changed, R package may be broken.
    But if test is integrated in this repository, we can found R package broken and fix before releasing.

  • Well synchronized documentation
    For example, In https://github.com/mlampros/RGF,
    Installation is started from git clone https://github.com/RGF-team/rgf.git.
    But pip installation is more easy.
    And many infomrmation about installation in this page https://github.com/RGF-team/rgf/tree/master/python-package .
    Integration makes it easier to erase duplicates and keep the latest documents.

  • Smooth support and review
    There are members familiar with RGF, so support will be smooth.
    There may be nobody who knows R in current RGF-Team, but reviews are possible to some extent (including document review).

@mlampros
Copy link
Collaborator Author

mlampros commented Jul 27, 2018

@fukatani I understand your concerns.

  • I agree that the R tests should be integrated to this repository, however I'm not familiarized with the process of including two programming languages (R, Python) in the same .travis.yml file
  • pip install is definitely more easy when someone intends to install a package. However, there are cases where installing from source is necessary (I experienced a similar case previous week when I attempted to install the newest version of rgf_python on my windows machine). I think that valuable information of installation procedures that a user can take advantage of should be kept from both README.md files in case that an integration of the repository should take place. In the last months I attempted many times to install Python packages (not only rgf_python) and as a user I think that it's very important to not consume too much time / effort in the installation process otherwise the probability that I drop the installation entirely increases
  • I guess that the support will be restricted to the classes or modules that the rgf_python offers as the RGF R package does nothing else than calling the methods of the rgf_python package. Till now the only issues that I encountered have been related with the reticulate package which provides the interface between Python and R. By that I mean the errors are mostly related to Python than to R.

@StrikerRUS
Copy link
Member

I think now we can move our discussion to the PR, because it'll be more effective than abstract words =)

@mlampros
Copy link
Collaborator Author

mlampros commented Jul 27, 2018

@StrikerRUS do I have to first fork the repository before I open the PR?

@StrikerRUS
Copy link
Member

StrikerRUS commented Jul 27, 2018

@mlampros
Yeah. I think the steps are:

  • fork rgf;
  • create folder R-package
  • copy-paste entire your repo in that folder
  • create a PR

I suppose, it's enough for starting point.

@StrikerRUS
Copy link
Member

@mlampros I gave a brief glance and have one general question:
is there any code style guide for R? To be more precise, are blank lines after each line with code encouraged? It's very hard to read such code...

@mlampros mlampros mentioned this issue Jul 27, 2018
@mlampros
Copy link
Collaborator Author

@StrikerRUS I think it depends on the coding style of each coder. However, It is allowed in R, and I am able to accomplish tasks better when I write code every second line.

@StrikerRUS
Copy link
Member

In general blank lines are used for separating logical blocks of code, but not each line of code. I see this for the first time 😮

@mlampros
Copy link
Collaborator Author

As I said before it depends on the coding style. I feel more comfortable when I live a blank line in between. Sorry, if I spoiled your day with my coding behaviour.

@mlampros
Copy link
Collaborator Author

I close this issue as it's no longer required (pull request 208).

@StrikerRUS
Copy link
Member

@mlampros Next time you can use this mechanism: https://help.github.com/articles/closing-issues-using-keywords/

@mlampros
Copy link
Collaborator Author

@StrikerRUS thanks I'll keep that in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants