Organized R Package from Volesti v1.1.2-6

[Soumya624]

Separated 'volesti' R package for v1.1.2-6 from 'cran_gen/cran_package' folder. And no errors were observed, while it's getting installed via RStudio.

[TolisChal]

@Soumya624 Thanks so much for this commit! The first big step to a brand new repository!
I left a few comments in some parts that are not clear to me.
In general this is great work!

[Soumya624]

@Soumya624 Thanks so much for this commit! The first big step to a brand new repository! I left a few comments in some parts that are not clear to me. In general this is great work!

Hi @TolisChal @vissarion ,

Thanks for the review. I have added a new commit in this PR with the submodule part. Currently 'src/include' is not there. Those C++ codes are coming through submodule. But, I had to change the header file paths for that. Please have a look. I think 'git pull --recurse-submodules' will work.

Thanks!

[vissarion]

Thanks! I have a few comments inline and a couple below.

1. Please remove all binaries from your PR (e.g. .o files).
2. Could you create a README that includes all the instructions needed to build the package? Such as, the submodule related commands.

[vissarion]

Thanks for the review. I have added a new commit in this PR with the submodule part. Currently 'src/include' is not there. Those C++ codes are coming through submodule. But, I had to change the header file paths for that. Please have a look. I think 'git pull --recurse-submodules' will work.

It returns the following error on my side

Submodule 'volesti' (https://github.com/GeomScale/volesti.git) registered for path 'volesti'
Cloning into '/workspace/Rvolesti/volesti'...
fatal: remote error: upload-pack: not our ref a0b26514f96004ac900c5fcb1c49a026443041eb
fatal: Fetched in submodule path 'volesti', but it did not contain a0b26514f96004ac900c5fcb1c49a026443041eb. Direct fetching of that commit failed.
fatal: 

[Soumya624]

a0b26514f96004ac900c5fcb1c49a026443041eb

Hi @vissarion ,

Thanks for the review and for pointing out this error. I think it's coming due to commit 'a0b26514f96004ac900c5fcb1c49a026443041eb'. I had slightly updated few headers on 'volesti v1.1.2-6' but it wasn't pushed. Updating in the latest PR!

[vissarion]

Thanks @Soumya624! I verify that the package can be installed and loaded locally.
However, I have a question regarding this commit Soumya624/volesti@a86be1f What is the reason of creating relative paths in includes? It is generally a bad practice. See also this PR GeomScale/volesti#244 that tries to fix/canonicalize include paths.

[Soumya624]

Thanks @Soumya624! I verify that the package can be installed and loaded locally. However, I have a question regarding this commit Soumya624/volesti@a86be1f What is the reason of creating relative paths in includes? It is generally a bad practice. See also this PR GeomScale/volesti#244 that tries to fix/canonicalize include paths.

Hi @vissarion ,
Tried to run it without by changing the relative path in include/convex_bodies/hpolytope.h. But got the following error. Any suggestions/feedbacks would be great!

[vissarion]

Thanks @Soumya624! I verify that the package can be installed and loaded locally. However, I have a question regarding this commit Soumya624/volesti@a86be1f What is the reason of creating relative paths in includes? It is generally a bad practice. See also this PR GeomScale/volesti#244 that tries to fix/canonicalize include paths.

Hi @vissarion , Tried to run it without by changing the relative path in include/convex_bodies/hpolytope.h. But got the following error. Any suggestions/feedbacks would be great!

This is probably some issue with your path. A quick remark is that in the CRAN directory structure the include directory (of volesti cpp code) is inside src while in your structure this is not the case.

Also as it is written in CRAN package structure policy all source files that are going to be compiled should be placed in srs. See https://cran.r-project.org/doc/manuals/R-exts.html#Package-structure

[Soumya624]

Hi @vissarion ,

Thanks for clarifying. Just a small follow-up. Should I need to update the CRAN directory structure? I can move the volesti directory inside the src.

[vissarion]

Hi @vissarion ,

Thanks for clarifying. Just a small follow-up. Should I need to update the CRAN directory structure? I can move the volesti directory inside the src.

Yes, please, but keep in mind that only the include directory is needed inside src not the whole volesti directory.

[Soumya624]

Hi @vissarion , @TolisChal ,

Added only the Include folder, inside src directory through Subtree. Also cleared the relative paths and updated absolute paths throughout. Please have a look!

[vissarion]

Please remove the objective .o files from src. Also please remove .Rbuildignore and .Rhistory directories.

[vissarion]

Other than the above comment the PR is now in good shape. Thanks @Soumya624!

@TolisChal are you OK with merging?

[Soumya624]

Hi @vissarion ,

Thanks for the review. Updated the suggested changes. Please take a look!

[vissarion]

I am OK with merging after addressing a couple of comments mentioned in the comments.

[TolisChal]

Very good work! All looks good to me, I'm approving it.

[Soumya624]

Hi @vissarion , @TolisChal ,

Thanks for the review. Updated the recently suggested changes. Would be great if you can have a look!

[vissarion]

Thanks!