-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
First pass at adding Rcpp::algorithm #428
Conversation
Do we need to template this to allow (at least) |
Actually the comment is from @nathan-russell |
It should work for int. Complex needs to be done yet.
|
Just for the sake of it, I'd like to say that if we decide to implement this, we can't say "well it doesn't handle X" so we can't bring it in. We have infinite flexibility to add things later on because this is a totally new API. |
@dcdillon This looks good. And regarding my comment in #426, the use of functors for algorithms that keep some sort of state between successive function calls is just the default approach I usually take in such situations, since it offers separation of the subroutine logic (i.e. what's done in the body of |
@@ -80,4 +80,6 @@ | |||
|
|||
#include <Rcpp/platform/solaris.h> | |||
#include <Rcpp/api/meat/meat.h> | |||
|
|||
#include <Rcpp/algorithms.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: a non-plural name is slightly preferred (e.g. <Rcpp/algorithm.hpp>
); just to be in-line with the STL <algorithm>
header. Also since the namespace name is algorithm
after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@kevinushey I'm on a little on the fence. If there were sufficient commitment and excitement for "sure, let's go off and reimplement all this" I'd be more inclined. Right now I see it as potentially disruptive for no real obvious gain (as we do have these functions in Rcpp Sugar). On the other hand by keeping it in a distinct sub-namespace it can't do too much damage. We could also develop this in a branch and see where it leads us. Overall somewhat ¯_(ツ)_/¯. I'll talk some more with @dcdillon over some hot or cold beverages. |
I changed the names of the functions and added a proof of concept as to how things could be templated as @kevinushey suggested. |
A quick response to @eddelbuettel regarding the utility of these type of functions. I came up with the idea when looking at an SO question regarding RcppParallel where someone was trying to use |
Right. That would be a good use case but we need to be very careful. The reason |
Anyway -- I really do like our deliberate approach here of discussion first. A welcome change. |
Yeah. I wouldn't have PR'ed at all, except without some code to talk about it's much harder to have a conversation. |
… problems with R interfaces
Sorry about the broken builds. My testing apparently wasn't giving me the results I needed. Anyhow, it got a little convoluted, but the end result is good. I created classes that take a c++ type as a parameter and, in turn, provide information about the R types. Specifically |
…er c++11 or later
Will revisit this after the next release. |
I wanted to bump this again, as it pertains directly to the fixes that just had to go in to Due to the way most of An iterator-based algorithm approach (where it makes sense) would alleviate these and also render the preponderance of duplicated code that currently exists in |
How do we get from here to there? Any alternative to incrementalism as we just did for |
|
OK. So because I'm stupid and deleted the fork that this PR came from, I have had to create a new one. Sorry..will have to close this one and open a new one to continue the work. |
This is a first pass at #426. It only does simple things and is intended as a simple example of what we can do with iterator based algorithms. I'd like to use this and get the discussion started.
Also, suggestions of other algorithms to implement can be added to this once we decide if we'd like to go forward with it.
@thirdwing - I ended up making a
helper
namespace to contain function objects. I think this addresses your comment in the issue conversation.