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

Get tracerer at rOpenSci #5

Closed
2 tasks done
richelbilderbeek opened this issue Apr 2, 2018 · 3 comments
Closed
2 tasks done

Get tracerer at rOpenSci #5

richelbilderbeek opened this issue Apr 2, 2018 · 3 comments

Comments

@richelbilderbeek
Copy link
Member

richelbilderbeek commented Apr 2, 2018

@richelbilderbeek
Copy link
Member Author

richelbilderbeek commented Jun 2, 2018

Feedback from @bjoelle:

[x] All the packages are missing package documentation (required by rOpenSci guidelines).

Added.

[x] Please also check devtools::spell_check, which shows some typos in documentation in all packages.

Added, fixed typos.

[x] calc_stderr_mean: this function asks for the sample_interval as input but doesn't seem to use it.
Done

[-] To simplify the code I would also suggest using this formula instead of the C code: stderr_mean = stats::sd(trace)/sqrt(length(trace)).

I added this suggestion as a test, validating the results are indeed identical.
Unexpectedly, the current and suggested calculation give different results. I do not know why
Tracer gives this different result, but I do know that tracerer intends to give the same
results as Tracer. Therefore, I choose to keep the old behavior, assuming the Tracer authors
have superior knowledge of the problem (as I only ported their code to C++).

[x] parse_beast_output_files, parse_beast_posterior: these functions need more detailed @return documentation

Added

[x] calc_summary_stats.R: the doc in this entire file refers to calculating ESS but this function calculates other things too: a list of all the stats calculated should be included in the @return doc. The @param traces doc for calc_summary_stats is also confusing ("a numeric vector or data frame or numeric vector")

Added

[x] All function need to have a @return documentation

Added

@richelbilderbeek
Copy link
Member Author

richelbilderbeek commented Jul 15, 2018

Feedback from @dwinter at the rOpenSci submission

Tracerer

It's a shame traceR was taken as a name on CRAN! (This name is fine, given the shorter one is not available). I only have minor comments about this one.

Minor comments of tracerer

In the package-level documentation consider referring the user to coda, a very widely-used package for MCMC analysis that includes diagnostics and plots that are not implemented here.

In calc_summary_stats the documentation for sample_interval make it clear this is the "sampling interval used in in the MCMC run" or similar (i.e. the sample_interval should match the run that produced the data, and is not used to further thin output in this function).

Consider exposing calc_hpd_interval to users. I can imagine users may sometimes one an 80% or 99% HPD for a particular value. At present
calc_summary_stats hard-codes a 95% HPD.

@richelbilderbeek
Copy link
Member Author

All problems are either closed or moved, thus closing this Issue.

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

No branches or pull requests

1 participant