search

BenoitMorel / AleRax

Gene tree - species tree reconciliation from gene tree distributions
21 stars 3 forks source link

Assertion false failed error in UndatedDTLMultiModel.hpp #16

Open jmayoral1 opened 6 days ago

jmayoral1 commented 6 days ago

Hello AleRax team,

Thank you for developing this neat tool!

I seem to have encountered an error when running AleRax (commit 636b535, git pulled on 11/27/24) using the "--transfer-constraint RELDATED" and "--infer-speciation-order" flags. After the "Initializing ccps and evaluators..." step, AleRax crashes and the following error message is printed out by each of the mpi workers: "UndatedDTLMultiModel.hpp:583: void UndatedDTLMultiModel::recomputeSpeciesProbabilities() [with REAL = double]: Assertion `false' failed."

AleRax runs fine on my dataset when ommitting the reldated and infer speciation order flags. And if I remove line 583 from UndatedDTLMultiModel.hpp and recompile AleRax, it seems to run properly when using the reldated and infer speciation order options.

Is it appropriate to remove that one line of code (583) in UndatedDTLMultiModel.hpp, or would this be incorrect?

I can provide more info specific to my run if needed.

Best, Josh

StefanFlaumberg commented 5 days ago

Hi Josh,

From my perspective as a user and code contributor (but not a team member):

The current version actually doesn't support any transfer constraint other than PARENTS. Yes, you can bypass/hack it by removing the assertion just like you mentioned, but it may result in wrong likelihood estimations. However, if you run with the --no-tl option, the likelihood difference should not be that significant.

The good news is that I've recently made a major code revision, the second part of which also fixes the reconciliation model files (including this problem). Hopefully, the revision will be accepted in a month or so (see PR BenoitMorel/AleRax#11). I haven't pushed the second part to GitHub yet, waiting for the first part to be accepted. But if you'd like to test it, I can publish the amendments in a separate branch of my AleRax fork in the coming days, please notify me.

Best, Stefan

jmayoral1 commented 5 days ago

Hi Stefan,

Thank you for the reply!

Sounds great, I'd be interested in testing your major code revision and seeing how it runs. Could you let me know/send a link to your AleRax fork when it is available?

I'll keep an eye on Noah's approval as well re: the PR #11 thread

Cheers, Josh

StefanFlaumberg commented 23 hours ago

Hi Josh,

I'm happy to inform you that the dev version with fixed reconciliation models can now be downloaded using the following command: git clone -b modeltest --recursive https://github.com/StefanFlaumberg/AleRax

There are no new features introduced, just bug fixing; the model complies with the original description in the AleRax paper. It is definitely not the final code version, but it should be mostly stable with respect to the output results. The only real issue remaining and to be fixed in the future is the high collision rate of the dated tree hash function, which may lead to somewhat suboptimal (yet still reasonable) results in the RELDATED mode. If you find any other serious problem, please feel free to report it :)

Best, Stefan

jmayoral1 commented 23 hours ago

Neat, thank you Stefan!

I'll keep you updated on any serious issues I encounter with this dev version. And I suppose I'll keep this issue open until we hear back from Noah about your proposed changes.

Cheers and thank you again for your contribution, Josh