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

Removing rngR #4

Merged
merged 1 commit into from Jul 25, 2017

Conversation

Projects
None yet
3 participants
@LeahPrice
Collaborator

LeahPrice commented Jul 25, 2017

RNGscope is established automatically with Rcpp attributes, so the unnecessary rng files have been removed.

Also fixing up the move selector function so that it is on the particle level rather than population level.

Removing rngR
RNGscope is established automatically with Rcpp attributes, so the unnecessary rng files have been removed.

Also fixing up the move selector function so that it is on the particle level rather than population level.
@eddelbuettel

Nice. Looks good to me.

Will wait on Adam to chime in too before merging.

@@ -551,6 +548,21 @@ namespace smc {
}
}
template <class Space>
void sampler<Space>::Multinomial(unsigned n, unsigned k, arma::vec w, unsigned int * X) {

This comment has been minimized.

@adamjohansen

adamjohansen Jul 25, 2017

Collaborator

It's been a long day, so apologies if I'm missing something, but do we even need this helper function? (By which I mean can we not just call rmultinom directly when it's used which I think is just 393/403 for resampling). It's not a big deal either way but less code is so often more...

@adamjohansen

adamjohansen Jul 25, 2017

Collaborator

It's been a long day, so apologies if I'm missing something, but do we even need this helper function? (By which I mean can we not just call rmultinom directly when it's used which I think is just 393/403 for resampling). It's not a big deal either way but less code is so often more...

This comment has been minimized.

@eddelbuettel

eddelbuettel Jul 25, 2017

Collaborator

I was wondering this too this morning (but have been out all day -- still vacationing). As I recall we now have a multinomial routine elsewhere, ie in Rcpp's rmulitnom wrapping R's own.

But we can keep these cleanups for another time.

@eddelbuettel

eddelbuettel Jul 25, 2017

Collaborator

I was wondering this too this morning (but have been out all day -- still vacationing). As I recall we now have a multinomial routine elsewhere, ie in Rcpp's rmulitnom wrapping R's own.

But we can keep these cleanups for another time.

This comment has been minimized.

@adamjohansen

adamjohansen Jul 25, 2017

Collaborator

Quite so -- cleanups can be done later -- I was just wondering if this was really needed and this seemed a good time to ask.

@adamjohansen

adamjohansen Jul 25, 2017

Collaborator

Quite so -- cleanups can be done later -- I was just wondering if this was really needed and this seemed a good time to ask.

This comment has been minimized.

@LeahPrice

LeahPrice Jul 26, 2017

Collaborator

Yeah I agree it would be better without that helper function. The Multinomial function uses rmultinom but it has a few extra lines to deal with the fact that uRSCount is of type arma::Col and rmultinom wants int *. I used uRSCount.memptr() to get type unsigned int * but I was having trouble casting to int *. Do you think it's an okay solution to just change uRSCount to type arma::Col? That's what I've tried and I'll do a pull request now for it. Feel free to reject it obviously if it's a bad solution.

@LeahPrice

LeahPrice Jul 26, 2017

Collaborator

Yeah I agree it would be better without that helper function. The Multinomial function uses rmultinom but it has a few extra lines to deal with the fact that uRSCount is of type arma::Col and rmultinom wants int *. I used uRSCount.memptr() to get type unsigned int * but I was having trouble casting to int *. Do you think it's an okay solution to just change uRSCount to type arma::Col? That's what I've tried and I'll do a pull request now for it. Feel free to reject it obviously if it's a bad solution.

This comment has been minimized.

@adamjohansen

adamjohansen Jul 26, 2017

Collaborator

You can't (really) cast unsigned* to int* because there's no guarantee that int and unsigned int have a closely related interpretation. You could use reinterpret_cast to tell the compiler to pretend that an unsigned* is an int* which would produce code that compiles... but it would have undefined behaviour.

The only reason for using an unsigned type was that it was a better logical description of a number of replicates. If there are pragmatic advantages for using a signed type, as there seem to be, then indeed that seems sensible.

(Don't feel that you have to respond to every question with new code, by the way, I was just curious as to whether it was really necessary -- and as Dirk said, there's no reason that small things like this can't be tidied up later if it's desirable to.)

@adamjohansen

adamjohansen Jul 26, 2017

Collaborator

You can't (really) cast unsigned* to int* because there's no guarantee that int and unsigned int have a closely related interpretation. You could use reinterpret_cast to tell the compiler to pretend that an unsigned* is an int* which would produce code that compiles... but it would have undefined behaviour.

The only reason for using an unsigned type was that it was a better logical description of a number of replicates. If there are pragmatic advantages for using a signed type, as there seem to be, then indeed that seems sensible.

(Don't feel that you have to respond to every question with new code, by the way, I was just curious as to whether it was really necessary -- and as Dirk said, there's no reason that small things like this can't be tidied up later if it's desirable to.)

This comment has been minimized.

@LeahPrice

LeahPrice Jul 26, 2017

Collaborator

Okay, in that case I should probably change RSIndices to type int in a future version too (I'm using uRSCount += arma::conv_to<arma::Col<int> >::from(RSIndices))

Thanks, I'll keep that in mind for future minor changes like this.

@LeahPrice

LeahPrice Jul 26, 2017

Collaborator

Okay, in that case I should probably change RSIndices to type int in a future version too (I'm using uRSCount += arma::conv_to<arma::Col<int> >::from(RSIndices))

Thanks, I'll keep that in mind for future minor changes like this.

@adamjohansen

This looks good to me, I've got little to say other than sorry for being a bit slow to get back to you.

@eddelbuettel eddelbuettel merged commit a40c781 into rcppsmc:master Jul 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@LeahPrice LeahPrice deleted the LeahPrice:RemovingRngR branch Jul 26, 2017

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