search

uqrmaie1 / admixtools

https://uqrmaie1.github.io/admixtools
71 stars 14 forks source link

Feasibility inconsistency between qpadm_rotate with full_results and without full_results #49

Open idaios opened 10 months ago

idaios commented 10 months ago

I have run qpadm_rotate with full_results TRUE and FALSE. After inspecting the results, I get the following output:

full_results = FALSE

  1. 1 <chr [4]> <chr [17]> 3 13 16.9 0.204 TRUE

full_results = TRUE 1 0000 0 13 16.9 2.04e- 1 3 0.179 2.57 -1.73 -0.0248 FALSE NA

obviously, the four weights are 0.179 2.57 -1.73 and -0.0248. Thus, results are not feasible, I guess. However, when full_results = FALSE, the report is TRUE for feasible.

uqrmaie1 commented 10 months ago

Thanks for letting me know about this. I'm not able to reproduce the problem, I get the same with full_results = FALSE and full_results = TRUE. Could you share with me the data and model for which you get the inconsistent results?

idaios commented 10 months ago

Dear Robert, thanks for the reply. I can't send you the raw data, probably I must get permission for this. However, I will send you the code and the RData file. I think with this you will be able to replicate what I have done. In fact you just need the arguments of the qpadm_rotate ( I call it two times, one with the full and one without the full option). Please check results # 546 (you will see I have commented it out).

Please let me know if you have any questions.. Meanwhile, I will rerun everything again, just to be sure that it's not some weird R artefact.

here is the wetransfer link

pavlos

On Wed, Sep 13, 2023 at 10:43 PM Robert Maier @.***> wrote:

Thanks for letting me know about this. I'm not able to reproduce the problem, I get the same with full_results = FALSE and full_results = TRUE. Could you share with me the data and model for which you get the inconsistent results?

— Reply to this email directly, view it on GitHub https://github.com/uqrmaie1/admixtools/issues/49#issuecomment-1718216777, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE553VUD7RIIIZ65XLYNGDX2IEGDANCNFSM6AAAAAA4WHJ4GM . You are receiving this because you authored the thread.Message ID: @.***>

--

Pavlos Pavlidis, PhD

uqrmaie1 commented 10 months ago

Thanks for sharing the data!

There was a bug that would only look at the weight of a single population for determining if a model is feasible, when running it with full_results = FALSE. That should be fixed now; full_results = FALSE and full_results = TRUE should always agree on whether any given model is feasible. Please let me know if you find any other issues!

idaios commented 10 months ago

Thanks Robert

Pavlos

idaios commented 10 months ago

Dear Robert, thanks a lot! I removed the link from the previous post.

Just one more question: where on the code was the bug? I checked the code myself yesterday before sending you the first message and I didn't find anything obviously wrong.

all the best pavlos

On Thu, Sep 14, 2023 at 1:26 AM Robert Maier @.***> wrote:

Thanks for sharing the data!

There was a bug that would only look at the weight of a single population for determining if a model is feasible, when running it with full_results = FALSE. That should be fixed now; full_results = FALSE and full_results = TRUE should always agree on whether any given model is feasible. Please let me know if you find any other issues!

— Reply to this email directly, view it on GitHub https://github.com/uqrmaie1/admixtools/issues/49#issuecomment-1718394834, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE553RO3Q5VCHKTRKIEP53X2IXLDANCNFSM6AAAAAA4WHJ4GM . You are receiving this because you authored the thread.Message ID: @.***>

--

Pavlos Pavlidis, PhD

idaios commented 10 months ago

ok, I checked the diff! thanks a lot!

pavlos

On Thu, Sep 14, 2023 at 11:52 AM Pavlos Pavlidis @.***> wrote:

Dear Robert, thanks a lot! I removed the link from the previous post.

Just one more question: where on the code was the bug? I checked the code myself yesterday before sending you the first message and I didn't find anything obviously wrong.

all the best pavlos

On Thu, Sep 14, 2023 at 1:26 AM Robert Maier @.***> wrote:

Thanks for sharing the data!

There was a bug that would only look at the weight of a single population for determining if a model is feasible, when running it with full_results = FALSE. That should be fixed now; full_results = FALSE and full_results = TRUE should always agree on whether any given model is feasible. Please let me know if you find any other issues!

— Reply to this email directly, view it on GitHub https://github.com/uqrmaie1/admixtools/issues/49#issuecomment-1718394834, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE553RO3Q5VCHKTRKIEP53X2IXLDANCNFSM6AAAAAA4WHJ4GM . You are receiving this because you authored the thread.Message ID: @.***>

--

Pavlos Pavlidis, PhD

--

Pavlos Pavlidis, PhD

uqrmaie1 commented 10 months ago

The problem was calling dplyr::between() on a 1-row matrix in qpadm_p(). There was a change in the behavior of between() somewhere between dplyr 1.0 and dplyr 1.1. Calling it on a matrix used to give the same result as calling it on a vectorized version of the matrix, but in dplyr 1.1, the 1-row matrix has to be converted to a vector first, otherwise it just considers the first column/element.