CompEvol / morph-models

Models for dealing with morphological data
GNU Lesser General Public License v2.1
7 stars 11 forks source link

The transition probability matrix in Lewis Mk model is incorrect #10

Open gavryushkina opened 6 years ago

gavryushkina commented 6 years ago

double should be used instead of integer for calculation line 108

dwbapst commented 6 years ago

Could this have had an impact on analyses with symmetric transition probabilities?

gavryushkina commented 6 years ago

It has an impact on any analysis using Lewis Mk model without specified frequencies. I am not quite sure which model you mean.

dwbapst commented 6 years ago

I was being confusingly specific. I just meant, would this likely have had an impact on previous analyses involving morphological datasets with BEAST2? Sounds like the answer is yes.

gavryushkina commented 6 years ago

Unfortunately yes. It should not be a big impact. I am checking my penguin analysis and so far there is only very little difference in morphological clock rate.

dwbapst commented 6 years ago

Well, I've found bugs like this in paleotree before, and had to alert people via social media/R-Phylo. Once your penguin re-analysis is done, will you be notifying users via the beast-users group of the issue and how much of an impact it made on your own re-analysis, so anyone who did an analysis recently knows to rerun their study?

gavryushkina commented 6 years ago

Yeah, I will need to notify users somehow.

On Fri, 29 Jun 2018 at 21:51 David Bapst notifications@github.com wrote:

Well, I've found bugs like this in paleotree before, and had to alert people via social media/R-Phylo. Once your penguin re-analysis is done, will you be notifying users via the beast-users group of the issue and how much of an impact it made on your own re-analysis, so anyone who did an analysis recently knows to rerun their study?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/CompEvol/morph-models/issues/10#issuecomment-401456916, or mute the thread https://github.com/notifications/unsubscribe-auth/AGJfciKPJcfA3q-3iTmrE5K2KX0reV_Iks5uBoVKgaJpZM4U5l7g .

-- Best regards, Dr. Alexandra "Sasha" Gavryushkina Postdoctoral fellow Department of Biochemisty University of Otago New Zealand

alexeid commented 6 years ago

Sorry to hear this, Sasha. The standard protocol is to make an announcement on beast-users forum detailing the bug and advising under what models and versions of the software people may have been affected.

I would recommend adding a unit test to check the transition probabilities for both equal and unequal frequencies. There should be some similar tests for the nucleotide models.

On 30/06/2018, at 8:02 AM, Alexandra Gavryushkina notifications@github.com wrote:

Yeah, I will need to notify users somehow.

On Fri, 29 Jun 2018 at 21:51 David Bapst notifications@github.com wrote:

Well, I've found bugs like this in paleotree before, and had to alert people via social media/R-Phylo. Once your penguin re-analysis is done, will you be notifying users via the beast-users group of the issue and how much of an impact it made on your own re-analysis, so anyone who did an analysis recently knows to rerun their study?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/CompEvol/morph-models/issues/10#issuecomment-401456916, or mute the thread https://github.com/notifications/unsubscribe-auth/AGJfciKPJcfA3q-3iTmrE5K2KX0reV_Iks5uBoVKgaJpZM4U5l7g .

-- Best regards, Dr. Alexandra "Sasha" Gavryushkina Postdoctoral fellow Department of Biochemisty University of Otago New Zealand — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/CompEvol/morph-models/issues/10#issuecomment-401459426, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3WSU0XHIMUDbfUz6Jzta_RGxz4uQYdks5uBofGgaJpZM4U5l7g.

dwbapst commented 6 years ago

Thanks for sending the comment to beast-users, @gavryushkina!