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

Ben weinstein #11

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

Ben weinstein #11

wants to merge 38 commits into from

Conversation

bw4sz
Copy link
Contributor

@bw4sz bw4sz commented Dec 4, 2016

  1. Updates to vignettes.
  2. Coded Vazquez 2009 mgen wrapper (descriptive method, no predict possible)
  3. Multinomial bayesian model with a Dirichlet prior, tested and runs clean.

bw4sz and others added 22 commits December 2, 2016 19:13
… cannot yet test the dmulti function for jags
…l bayesian models, i clearly don't understand how paths are exporting when rstudio makes a package. I had to hard code the paths, i'm on the airplane, can't google. Clearly this needs to be address, including the html markdown doc i made to show off the method that is loitering in the R/ folder, where do tutorials go?
… functions work with bayes Intercept and Binomial models
…ion, exports the documentation, still fails description file check in rstudio
…w paths are export using kevin's export function
@SteveViss SteveViss added this to the Ben's magic milestone Dec 4, 2016
@codecov-io
Copy link

codecov-io commented Dec 5, 2016

Current coverage is 30.16% (diff: 27.41%)

Merging #11 into master will decrease coverage by 5.64%

@@             master        #11   diff @@
==========================================
  Files            16         18     +2   
  Lines           377        537   +160   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            135        162    +27   
- Misses          242        375   +133   
  Partials          0          0          

Powered by Codecov. Last update c7cc5d3...426be82

@tpoisot
Copy link

tpoisot commented Dec 5, 2016

@bw4sz this needs to be tested before merging.

Copy link

@tpoisot tpoisot left a comment

Choose a reason for hiding this comment

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

Need tests

@bw4sz
Copy link
Contributor Author

bw4sz commented Dec 5, 2016

@tpoisot

Can you provide a bit more detail? When it says the code coverage has decreased, does it mean that some dependencies are undefined, or that there are global variables missing? I'm not totally following what is being evaluated.

@KevCaz
Copy link
Member

KevCaz commented Dec 5, 2016

As an example I add a test for OccupancyToJags in the previous commit !
I simply test that the file is indeed created.

@tpoisot
Copy link

tpoisot commented Dec 5, 2016

@bw4sz sure -- it's counting the number of lines that are executed when you run the tests. It's expected that every possible situation is tested. For example the test by @KevCaz is one, but it's not sufficient since it's not telling anything about whether the file is correct.

@KevCaz
Copy link
Member

KevCaz commented Dec 5, 2016

Tim is right, what I have done is not enough. I guess the best is to apply your models (which requires jags files) on simple cases for which results are known.

@SteveViss
Copy link
Contributor

The test implemented by @KevCaz for the functions (binomialToJags, interceptToJags, multinomialToJags, poissonToJags, OccupancyToJags) are fine. These functions are supposed to create files.

@bw4sz, is there a way to send a fake dat object to your fit_bayesreg function and know (a priori) the expected results/values (or at least the range) of this function? In that case, you compare the output of the function fit_bayesreg with the expected result.

If you need further details or examples have a look at http://r-pkgs.had.co.nz/tests.html. We know that implementing unit tests is more jobs but that practice is well suitable to prevent bugs and errors in the code.

@KevCaz
Copy link
Member

KevCaz commented Dec 6, 2016

Looking at the names of these functions, I wonder: shall we start the name by a lowercase letter or a uppercase one?

@SteveViss
Copy link
Contributor

@KevCaz, lower !

@bw4sz
Copy link
Contributor Author

bw4sz commented Dec 21, 2016

Looks like steve updated the camelCase, and the namespace will need to be rebuilt. I will not get to testing these functions this week, i suggest we move forward with getting functions together.

@KevCaz
Copy link
Member

KevCaz commented Dec 21, 2016

Ben, I have stried to solve the issues, can you check if everything is Ok on your side??

@bw4sz
Copy link
Contributor Author

bw4sz commented Jan 3, 2017

@KevCaz , i pulled, merged, and pushed, everything looks okay on first pass. Will run my vignette in a sec.

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

Successfully merging this pull request may close these issues.

5 participants