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

Feature request: add offset #130

Open
joelnitta opened this issue Aug 16, 2021 · 14 comments
Open

Feature request: add offset #130

joelnitta opened this issue Aug 16, 2021 · 14 comments
Labels
volunteer needed Need a volunteer to confirm

Comments

@joelnitta
Copy link

Is your feature request related to a problem? Please describe.

The offset is required for specifying the date of a fossil calibration point, which is a major use-case of BEAST. Currently it is not available in beautier.

Describe the solution you'd like

The create_distr() family of functions should have an argument offset that allows setting the offset.

Describe alternatives you've considered

Editing the XML file afterwards either programmatically or by hand.

Additional context

Since create_distr() functions get used in other contexts besides specifying mrca_prior, it may be that the offset setting doesn't make sense / wouldn't get used in all cases.

Example

Example created using Primates.nex example file of BEAST2:

  • Screenshot of setting

offset_image

@richelbilderbeek
Copy link
Member

Thanks @joelnitta for this awesome feature request, it would be hard to top that!

AFAICS, I will likely be able to do this before Friday 5th of September, as I first want to get babette on CRAN again.

Can I assume you volunteer to test this new feature? It saves other users a lot of frustration :-)

If you are eager to do some coding yourself, I can write the tests that need to pass; you'll be added to DESCRIPTION if you choose this option.

Again, thanks for the awesome work and cheers, Richel

@joelnitta
Copy link
Author

No problem; the template was very helpful for drafting the issue!

I can try running the feature once its ready, but unfortunately I can't get into the code at the moment. I'm already down another BEAST rabbit-hole...

No rush on this - please focus on getting babette on CRAN again.

@richelbilderbeek
Copy link
Member

Hi @joelnitta, thanks: someone to check my work is the minimum I need!

You could help me a lot sharing the FASTA file for your excellent Issue: I will then simply write a test to fix what you suggested there. Would that be possible?

I will keep you in the loop when I work on it, approx within 3 weeks!

@joelnitta
Copy link
Author

I don't have a FASTA file beyond what I mentioned in the issue. I just used the Primates.nex file that comes with the BEAST2 examples (same one you use in your examples):

fasta_filename <- "primates.fas"
save_nexus_as_fasta(
  nexus_filename = beastier::get_beast2_example_filename("Primates.nex"),
  fasta_filename = fasta_filename
)

Actually, as far as testing... what about adding another example repo (similar to https://github.com/richelbilderbeek/babette_example_1) showing the analysis result after running with one MRCA prior specified with an offset. It should be very clear from the tree if it worked or not: the specified node should not be any younger than the offset.

@richelbilderbeek
Copy link
Member

Hi @joelnitta, thanks for sharing the source of the FASTA file!

For testing: all I ask is to run your (or I even supply that as well) code on a new version of beautier on your computer, to see if everything still works:

# Installs the newer version of beautier
remotes::install_github("ropensci/beautier", ref = "develop")

# Run your code here

As you can see: nothing fancy. Just asking me to do so saves a lot of headaches to other users. Would you be up for that?

@richelbilderbeek
Copy link
Member

Hi @joelnitta, I checked the XML file that you uploaded. It contains a BEAST2 setup with multiple tree priors. babette does not support multiple tree priors yet and is not scheduled to do so anytime soon.

Would adding an offset still be useful in your use-case, if it is limited to one tree prior?

@joelnitta
Copy link
Author

joelnitta commented Aug 18, 2021

Actually, the multiple tree priors was an accident - I think I used a different BEAST example nexus file by mistake. Sorry for the confusion!

But that brings up a good point: the offset is really only helpful if I am able to define multiple MRCA priors. Most dating analyses rely on >1 fossil calibration point. I was considering filing that as a separate issue, but I figured the offset would need to be implemented in the case of just one MRCA first.

@joelnitta
Copy link
Author

I edited the gist, now it is actually based off the fasta file (not the original BEAST nexus file)

https://gist.github.com/joelnitta/16155f7831cb147e82842159cd7733c8

@richelbilderbeek
Copy link
Member

But that brings up a good point: the offset is really only helpful if I am able to define multiple MRCA priors. Most dating analyses rely on >1 fossil calibration point. I was considering filing that as a separate issue, but I figured the offset would need to be implemented in the case of just one MRCA first.

Adding multiple priors would take me a full-time month (i.e. working hours), so without external forces (funding or people), this seems unlikely to happen.

Adding that offset seems doable in my spare time. Would it be useful to you, even with one prior?

@joelnitta
Copy link
Author

joelnitta commented Aug 19, 2021

It would be somewhat useful as I could then write additional scripts (using the xml2 package) to loop over MRCA nodes and add them to the XML. TBH my use case has since changed somewhat and I'm now considering using BEASTv1, so this isn't such a high priority for me personally anymore. So it's up to you - fine with me to leave this open until there is more bandwidth to implement multiple MRCAs in babette.

[edit by @richelbilderbeek: note that #131 is a feature request for multiple MRCA priors]

@richelbilderbeek
Copy link
Member

Hi @joelnitta, in that case, I will leave this (excellent!) Issue open until I find a new user. Good luck with your research!

@evolucionario
Copy link

Not urgent right now as I managed to add an 'offset' option to the gamma_distr_to_xml() function but it would be nice to add this option in the main package. Otherwise, most calibration densities are practically useless.

@evolucionario
Copy link

evolucionario commented Jan 3, 2022

[edit: richelbilderbeek moved this Feasture Request to #131]

@richelbilderbeek richelbilderbeek changed the title Add offset Feature request: add offset Jan 4, 2022
@richelbilderbeek
Copy link
Member

Due to releasing a new CRAN version, I paste the test (that needs beastier) here:

test_that("Offset", {

  skip("Issue #130. https://github.com/ropensci/beautier/issues/130")
  # To reproduce 'distr_offset.xml' we need support for multiple tree priors
  # first

  fasta_filename <- beautier::get_beautier_tempfilename(
    pattern = "primates_",
    fileext = ".fas"
  )
  save_nexus_as_fasta(
    nexus_filename = beastier::get_beast2_example_filename("Primates.nex"),
    fasta_filename = fasta_filename
  )

  created <- create_beast2_input_from_model(
    input_filename = fasta_filename,
    inference_model = create_inference_model(),
    beauti_options = create_beauti_options_v2_6()
  )
  expected <- readLines(get_beautier_path("distr_offset.xml"))
  stringr::str_subset(expected, "offset")

  compare_lines(
    lines = created,
    expected = expected,
    created_lines_filename = "~/created.xml",
    expected_lines_filename = "~/expected.xml"
  )
  expect_true(are_equivalent_xml_lines(created, expected, verbose = TRUE))
  file.remove(fasta_filename)
})

@richelbilderbeek richelbilderbeek added the volunteer needed Need a volunteer to confirm label May 31, 2022
richelbilderbeek pushed a commit that referenced this issue May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
volunteer needed Need a volunteer to confirm
Projects
None yet
Development

No branches or pull requests

3 participants