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

refine w/o TreeTime Error #348

Closed
emmahodcroft opened this issue Aug 9, 2019 · 3 comments
Closed

refine w/o TreeTime Error #348

emmahodcroft opened this issue Aug 9, 2019 · 3 comments

Comments

@emmahodcroft
Copy link
Member

This was introduced inadvertently because of concurrent development in refine in #282 and #283 that meant the two fixes were never tested together :)

#282 allows trees to be re-rooted without time-inference, but checks that the rooting instructions aren't Treetime arguments ("best", "min_dev", etc) (line 210-211, refine.py).

However, if you are running refine without time-inference (as I was working on in #283 ) and without the --root argument (just wanting internal node names), there's a default value for --root of "best" - so this check fails.

I am not keen to just get rid of the default being "best" for --root because this would change the behaviour of runs without any warning (and probably be hard to catch), if users have runs where they don't specify --root and are expecting it to default to "best".

I wouldn't even be in favour of a warning, really, since telling users "you specified 'best' and we are going to ignore it" make 0 sense when they didn't actually specify a root at all...

Perhaps instead, if there's no --timetree argument and args.root=='best', this is ignored and the run proceeds, and we could say something like "Adding internal node names without rerooting" (or "Adding internal node names with rerooting" if it's been specified correctly) to make the situation clear but unsurprising to users.

Happy to implement this if that sounds good, but wanted to check with @rneher & @PierreBarrat that this works alongside their original intention.

@rneher
Copy link
Member

rneher commented Aug 9, 2019

I would probably raise an error if something other than the default is specified and warn otherwise (mentioning that it was likely by default).

@emmahodcroft
Copy link
Member Author

Worth noting that if you use --keep-root this solves this. But not sure we should have to require this. Will make a PR.

@emmahodcroft
Copy link
Member Author

Closed by #349

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants