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

Switching to a population level object #2

Merged
merged 4 commits into from Jul 24, 2017

Conversation

Projects
None yet
3 participants
@LeahPrice
Collaborator

LeahPrice commented Jul 22, 2017

Instead of having a particle level object, this version uses a population level object with values stored in an std::vector and log weights stored in an arma::vec.

An MCMC step was also added to Initialise().

Switching to a population level object
Instead of having a particle level object, this version uses a population level object with values stored in an std::vector and log weights stored in an arma::vec.

An MCMC step was also added to Initialise().
@eddelbuettel

eddelbuettel requested changes Jul 22, 2017 edited

This looks pretty good!

The Travis CI run failed, and that is partly my fault as the setup is fairly rigid. The package now needs RcppArmadillo, and we have to add this.

That is as easy as:

1 file changed, 1 insertion(+), 1 deletion(-)
.travis.yml | 2 +-

modified   .travis.yml
@@ -13,7 +13,7 @@ before_install:
   - ./run.sh bootstrap
 
 install:
-  - ./run.sh install_aptget r-cran-rcpp 
+  - ./run.sh install_aptget r-cran-rcpp r-cran-rcpparmaillo
   
 script:
   - ./run.sh run_tests

Can you please try this?

@LeahPrice

This comment has been minimized.

Show comment
Hide comment
@LeahPrice

LeahPrice Jul 22, 2017

Collaborator

Sure, thank you for the lines to fix this! It looks like it passed this time.

Collaborator

LeahPrice commented Jul 22, 2017

Sure, thank you for the lines to fix this! It looks like it passed this time.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 22, 2017

Collaborator

I think with that we're good. Let's wait for @adamjohansen to take a peek at it, but I think we got this one covered now. Yay!

(As for going forward: go to the travis-ci.org website and assign your rcppsmc (and/or other repos you want tested. You need to turn this one at a time for new repos. Forks usually work as is, but because you were not set up before it did not automagically work. This is hugely worth it so I recommend you look into it.)

Collaborator

eddelbuettel commented Jul 22, 2017

I think with that we're good. Let's wait for @adamjohansen to take a peek at it, but I think we got this one covered now. Yay!

(As for going forward: go to the travis-ci.org website and assign your rcppsmc (and/or other repos you want tested. You need to turn this one at a time for new repos. Forks usually work as is, but because you were not set up before it did not automagically work. This is hugely worth it so I recommend you look into it.)

@LeahPrice

This comment has been minimized.

Show comment
Hide comment
@LeahPrice

LeahPrice Jul 22, 2017

Collaborator

Cool, thank you! Yeah it'll be nice to not get a surprise failure with Travis CI when I do future pull requests.

Collaborator

LeahPrice commented Jul 22, 2017

Cool, thank you! Yeah it'll be nice to not get a surprise failure with Travis CI when I do future pull requests.

@eddelbuettel

There are a few small things (indentation etc) in here which we can clean up later.

LeahPrice added some commits Jul 24, 2017

Updating copyright headers
and removing particle.h because it was replaced by population.h in a previous commit.
Rcpp attributes and example namespaces
Using Rcpp attributes, using a namespace for each example and switching to armadillo vectors where possible in the examples.
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 24, 2017

Collaborator

Thanks for the additional commits. I think it might be best to just hold off committing until @adamjohansen is back online (which should be today) and we can finalize this.

Collaborator

eddelbuettel commented Jul 24, 2017

Thanks for the additional commits. I think it might be best to just hold off committing until @adamjohansen is back online (which should be today) and we can finalize this.

@adamjohansen

This looks great to me. Thanks both of you and apologies for the slow reply...

@LeahPrice

This comment has been minimized.

Show comment
Hide comment
@LeahPrice

LeahPrice Jul 24, 2017

Collaborator

Also sorry Adam, I for some reason thought that my commits today wouldn't show up on this pull request. I hope this wasn't too annoying for you to review.

Collaborator

LeahPrice commented Jul 24, 2017

Also sorry Adam, I for some reason thought that my commits today wouldn't show up on this pull request. I hope this wasn't too annoying for you to review.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 24, 2017

Collaborator

I for some reason thought that my commits today wouldn't show up on this pull request

You could revert these commits. Not yet saying you should, just saying you could.

If you want commits to be somewhere else, create a branch first. What happened here is perfectly logical and a feature -- one wants to be able to add to / augment a pull request.

Collaborator

eddelbuettel commented Jul 24, 2017

I for some reason thought that my commits today wouldn't show up on this pull request

You could revert these commits. Not yet saying you should, just saying you could.

If you want commits to be somewhere else, create a branch first. What happened here is perfectly logical and a feature -- one wants to be able to add to / augment a pull request.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 24, 2017

Collaborator

But I think we're good. here. Let me merge this now, and then we can do smaller / simpler follow-up PRs.

Collaborator

eddelbuettel commented Jul 24, 2017

But I think we're good. here. Let me merge this now, and then we can do smaller / simpler follow-up PRs.

@eddelbuettel eddelbuettel merged commit 134a9cf into rcppsmc:master Jul 24, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 24, 2017

Collaborator

@LeahPrice One thing you want to look into is defining your username and email:

*   134a9cf - (HEAD -> master, origin/master, origin/HEAD) Merge pull request #2 from LeahPrice/PopulationLevel (36 seconds ago) <Dirk Eddelbuettel>
|\
| * 43f3a63 - Rcpp attributes and example namespaces (8 hours ago) <Unknown>
| * 8ef8afd - Updating copyright headers (8 hours ago) <Unknown>
| * 7eca96b - Adding RcppArmadillo to installs (2 days ago) <Unknown>
| * 834d3c7 - Switching to a population level object (2 days ago) <Unknown>
|/
* 63bb33d - small update for registration (4 days ago) <Dirk Eddelbuettel>

"Unknown" is not the type of credit you want in the long run.

Collaborator

eddelbuettel commented Jul 24, 2017

@LeahPrice One thing you want to look into is defining your username and email:

*   134a9cf - (HEAD -> master, origin/master, origin/HEAD) Merge pull request #2 from LeahPrice/PopulationLevel (36 seconds ago) <Dirk Eddelbuettel>
|\
| * 43f3a63 - Rcpp attributes and example namespaces (8 hours ago) <Unknown>
| * 8ef8afd - Updating copyright headers (8 hours ago) <Unknown>
| * 7eca96b - Adding RcppArmadillo to installs (2 days ago) <Unknown>
| * 834d3c7 - Switching to a population level object (2 days ago) <Unknown>
|/
* 63bb33d - small update for registration (4 days ago) <Dirk Eddelbuettel>

"Unknown" is not the type of credit you want in the long run.

@LeahPrice

This comment has been minimized.

Show comment
Hide comment
@LeahPrice

LeahPrice Jul 24, 2017

Collaborator

Okay cool, thanks. I still haven't fixed that comment for the function so I'll do that in another pull request then.

Collaborator

LeahPrice commented Jul 24, 2017

Okay cool, thanks. I still haven't fixed that comment for the function so I'll do that in another pull request then.

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

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