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

Removing Multinomial helper function #5

Merged
merged 1 commit into from
Jul 26, 2017

Conversation

LeahPrice
Copy link
Collaborator

@LeahPrice LeahPrice commented Jul 26, 2017

Removing the Multinomial helper function and changing uRSCount to type arma::Col.

@@ -71,7 +71,7 @@ namespace smc {
///Structure used internally for resampling.
arma::vec dRSWeights;
///Structure used internally for resampling.
arma::Col<unsigned int> uRSCount;
arma::Col<int> uRSCount;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A trivial detail, but the u prefix denotes an unsigned value...

Now this is based on some pretty old code and many don't like Hungarian notation any more. Which is fine, it's not something I'm religious about, but if we use it or something that looks like it then it should be meaningful. nRSCount would better match an object of this sort, but as soon as we're using arma:Col rather than a simple array even that's a bit of a stretch.

With that in mind, it would be good to think about how best to refer to name this one object to avoid introducing any unnecessary confusion and at some later stage (not necessarily within GSOC and this isn't your problem so much as an RcppSMC one) to think a bit about variable naming conventions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's good to know. Unfortunately I don't think this is the only variable name I've done this with.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't worry too much, I think most people would curse me more for introducing the convention in the first place. It would be good to try at least to be consistent within the code, but there's absolutely no urgency.

@adamjohansen
Copy link
Collaborator

This looks fine to me -- modulo a slightly pedantic quibble over variable naming which is largely a hangover from my own slightly arcane approach to naming variables in an earlier project -- thanks for the quick update...

@eddelbuettel
Copy link
Collaborator

I don't think this is the only variable name I've done this with.

In general, it is common practice to NOT change other's people's preferred indentation, naming (camelCase versus snake_case versus hungarian versus....) if that is the sole change.

It's better to focus on the big picture. I'll open a ticket so that we can set up a better plan.

Also: arma::Col<unsigned> is IIRC the same as arma::uvec which is more concise. We generally don't use the explicitly templated form.

@eddelbuettel eddelbuettel merged commit fdbb46d into rcppsmc:master Jul 26, 2017
@LeahPrice
Copy link
Collaborator Author

Yes I didn't intentionally change those things as I don't know enough about them to have a preference (and especially didn't when first starting!). I will focus on the things I've put in the plan, but it should be very quick to fix up that latter comment (maybe along with one of my upcoming pull requests).

@LeahPrice LeahPrice deleted the RemoveMultinHelper branch July 27, 2017 12:38
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

Successfully merging this pull request may close these issues.

3 participants