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 babette at rOpenSci #9

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

Get babette at rOpenSci #9

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 @bjoelle:

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

  • Added.

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

  • Added, fixed typos.

this package needs a clearer statement of need (as per rOpenSci guidelines): some of the text from the onboarding issue and the description would fit nicely.

  • Put this in R/babette.R

bbt_run: the @return documentation is missing.

  • Added.

bbt_run: The default behaviour is to place the output in temporary files which are deleted at the end of the run. I would add a warning about that in the doc or in the code, as I don't think this would be expected by most users.

  • Added.

bbt_run: the doc should mention that rng_seed can be NA and what happens in that case (i.e the seed will be set to the BEAST2 default 127).

  • Added.

bbt_run.R: typos l119 "Must have as many FASTA filenames as BEAST2 output trees filenames"

  • Fixed typo

@richelbilderbeek
Copy link
Member Author

richelbilderbeek commented Jul 7, 2018

Feedback @dwinter:

Although the need for these packages is very real, the README file on the babette does not currently explain this. A clear statement of purpose (similar to the one given in the submission post) would be helpful.

  • Also bjoelle thought so, put this in R/babette.R

For the most part, the code in each package is well-written, appropriately modular meets the rOpenSci guidelines. So I think the packages will be very useful for researchers and a good addition to the rOpenSci project.

I do have some concerns about the package, namely

(a) That the code used to specify run-parameters in beautier is complex and may prove difficult to maintain or extend as a result.

  • Explanation here:

bjoelle and dwinter share this same thought.

About complexity: as babette has a user interface as flexible and complex as BEAUti, I think the user interface cannot be dumbed down.

About extendibility: I designed babette with extendability in mind. Being familiar with C++ software architecture (e.g. John Lakos, Robert C. Martin, to name a few), I opted for a C-style software architecture, which is the C version of using objects, inheritance and polymorphism. You can see its signature often:

if (is_a(x)) { 
  process_a(x)) 
} else if (is_b(x)) { 
  process_b(x)) 
} else {
  testit::assert(is_c(x))
  process_c(x)
}

If now a d gets added, babette will detect this and inform where code needs to be added. As most objects are independent (e.g. a site model has no interaction with a tree prior), adding new functionality is expected to take constant time.

(b) The user-exposed functions in this package are all quite "low-level", creating a daunting interface for setting prior distributions and preparing files for analysis.

This latter concern may be partly addressed by providing a more "narrative" vignette to supplment the examples in exiting ones. I provide more detail on these concerns and point out minor problems for each package below.

  • Add a tutorial vignette

@richelbilderbeek
Copy link
Member Author

Processed all feedback, posted it at rOpenSci. Will close this Issue somewhen August if no replies on this.

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