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

CRAN submisssion? #3

Open
rsbivand opened this issue Mar 5, 2021 · 10 comments
Open

CRAN submisssion? #3

rsbivand opened this issue Mar 5, 2021 · 10 comments

Comments

@rsbivand
Copy link

rsbivand commented Mar 5, 2021

@hunzikp Could I please ask whether this package is being considered for submission to CRAN? With the BH version patch from @AB-Kent in the @akoyabio fork if possible? Motivation: faster finding of spatial point neighbours.

@AB-Kent
Copy link
Collaborator

AB-Kent commented Mar 25, 2021

I'm also interested in having an rtree package on CRAN. @rsbivand does it make sense to submit my fork?

@rsbivand
Copy link
Author

@AB-Kent I'm unsure about whether it is OK to take over a source package without the acceptance of the authors. For spdep, I chose to go with dbscan replacing RANN, as frNN() does a reasonable job, and extends to 3D:

> set.seed(1)
> xy <- cbind(runif(100000), runif(100000))
> d <- 0.001
> library(rtree)
> system.time({xy_rtree <- RTree(xy); wd_ls <- withinDistance(xy_rtree, xy, d)})
   user  system elapsed 
  0.088   0.000   0.088 
> system.time(wd_ls1 <- lapply(wd_ls, sort))
   user  system elapsed 
  0.669   0.000   0.671 
> system.time(wd_ls2 <- lapply(1:length(wd_ls1), function(i) {x <- wd_ls1[[i]]; x[x != i]}))
   user  system elapsed 
  0.078   0.000   0.078 
> library(dbscan)
> system.time(fr_xy <- frNN(xy, eps=d)$id)
   user  system elapsed 
  2.407   0.000   2.412 
> system.time(fr_xy1 <- lapply(fr_xy, sort))
   user  system elapsed 
  1.779   0.000   1.783 
> all.equal(wd_ls2, fr_xy1)
[1] TRUE

As you can see, rtree would accelerate finding points within distances a lot, even compared to dbscan. The advantages are large for larger numbers of points. Could you please try to contact the authors outside github?

@AB-Kent
Copy link
Collaborator

AB-Kent commented Mar 25, 2021

@rsbivand Yes I will try to contact @hunzikp.

@hunzikp
Copy link
Owner

hunzikp commented Apr 25, 2021

Perfectly ok with me! (Sorry for the slow turn-around - I hope this is still helpful for you folks).

Cheerio - hunzikp

@AB-Kent
Copy link
Collaborator

AB-Kent commented Apr 26, 2021

That's great @hunzikp, thank you! I'm not sure the best way to proceed. Probably the simplest thing would be for you to make me (AB-Kent) an admin on this repository. Then I will merge in the changes in my fork and transfer ownership to my company account (akoyabio).

@AB-Kent
Copy link
Collaborator

AB-Kent commented May 10, 2021

@rsbivand I am working on a CRAN submission for this package. This is my first CRAN package and there is an issue I hope you can advise me on.

It doesn't seem to be possible to make a package that will pass CMD CHECK without WARNINGs on Windows. There is one WARNING due to the below compiler warnings.

These warnings come from the Boost headers. The Boost maintainers have evaluated them, consider them false positives, and used #pragmas to disable the warnings; see here:
boostorg/container@6504af8

The maintainer of the BH R package chose to re-enable the compiler warnings by commenting out the #pragmas here:
eddelbuettel/bh@5418216

As a result my code generates false-positive warnings, shown below.

If I add the #pragma to my code to disable the compiler warning, that generates a different CMD CHECK WARNING about disabling compiler warnings. So I either have the compiler warning or the #pragma WARNING.

Do you think these WARNINGs will prevent the package from being accepted on CRAN? Do you have any idea which WARNING is preferable and more likely to be accepted?

Thank you for any help,
Kent

Here is the actual compiler warning:

Found the following significant warnings:

D:/a/_temp/Library/BH/include/boost/container/detail/copy_move_algo.hpp:183:19: warning: 'void* memmove(void*, const void*, size_t)' writing to an object of type 'value_type' {aka 'struct std::pair<boost::geometry::model::point<double, 2, boost::geometry::cs::cartesian>, __gnu_cxx::__normal_iterator<const std::pair<boost::geometry::model::point<double, 2, boost::geometry::cs::cartesian>, unsigned int>*, std::vector<std::pair<boost::geometry::model::point<double, 2, boost::geometry::cs::cartesian>, unsigned int> > > >'} with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Wclass-memaccess]

D:/a/_temp/Library/BH/include/boost/container/detail/copy_move_algo.hpp:212:19: warning: 'void* memmove(void*, const void*, size_t)' writing to an object of type 'boost::movelib::detail::iterator_to_element_ptr<std::pair<boost::geometry::model::point<double, 2, boost::geometry::cs::cartesian>, __gnu_cxx::__normal_iterator<const std::pair<boost::geometry::model::point<double, 2, boost::geometry::cs::cartesian>, unsigned int>, std::vector<std::pair<boost::geometry::model::point<double, 2, boost::geometry::cs::cartesian>, unsigned int> > > >>::element_type' {aka 'struct std::pair<boost::geometry::model::point<double, 2, boost::geometry::cs::cartesian>, __gnu_cxx::__normal_iterator<const std::pair<boost::geometry::model::point<double, 2, boost::geometry::cs::cartesian>, unsigned int>*, std::vector<std::pair<boost::geometry::model::point<double, 2, boost::geometry::cs::cartesian>, unsigned int> > > >'} with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Wclass-memaccess]

@rsbivand
Copy link
Author

Which branch of which repo? I have https://github.com/akoyabio/rtree/tree/master currently, will try with Windows if I get a chance tomorrow. I think that if the #pragmas are removed in the BH package, that is correct.

@AB-Kent
Copy link
Collaborator

AB-Kent commented May 10, 2021

Yes, that is the correct repo and branch. You can see build results from GitHub Actions here: https://github.com/akoyabio/rtree/runs/2513098433?check_suite_focus=true
Some discussion on the Rcpp list starting here:
http://lists.r-forge.r-project.org/pipermail/rcpp-devel/2021-May/010602.html

@rsbivand
Copy link
Author

Yes, I see the same on Windows. I'm unsure why this might be.

00check.log
00install.zip

The advice from Dirk and Iñaki is as good as you can get, these are things they really know most about. I think explaining to CRAN that the warnings reflect a less mature part of Boost may help, when the submission is examined, and probably in the initial submission form too, to alert CRAN to the need for an exception if possible.

@AB-Kent
Copy link
Collaborator

AB-Kent commented Jul 21, 2021

I am trying again with CRAN submission. I have un-forked akoyabio/rtree and will use that as the primary repo from now on.

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

3 participants