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

Add & clarify rooting options #263

Merged
merged 2 commits into from May 6, 2019

Conversation

Projects
None yet
5 participants
@emmahodcroft
Copy link
Contributor

commented Feb 14, 2019

Along with @rneher's recent changes/clarification of treetime's rooting options, this should make options more accessible and easier to understand in augur refine.

Rooting options in help have been updated to match treetime, and defaults made clearer. --keep-root allows users to prevent rerooting, and --covariance/no-covariance allows users to turn this on/off in a clearer way.

By default most users will have been using rooting methods that account for covariation (though probably most didn't realise it), so to avoid changing expected behaviour this is by default 'on' with the new explicit flag, and is turned 'off' with --no-covariance.

@emmahodcroft emmahodcroft requested a review from rneher Feb 14, 2019

@trvrb

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

Thanks @emmahodcroft. This looks good. I just have two small thoughts on interface:

  1. Maybe get rid of --covariance option. It seemed confusing to me to have a store_true argument that defaults to true. I think better to just have --no-covariance as an option and this makes it clear that covariance is the default.

  2. I find it strange that --root and --keep-root are separate options with --keep-root overriding. I think cleaner would be to just have an option of --root none that makes it so that no rerooting is done. (Though I do see how you were trying to align augur --root options 1:1 with TreeTime --reroot options.)

@emmahodcroft

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Hi Trevor, Thanks for your comments.

  1. With --covariance/--no-covariance I was trying to stick to what argparse supports well (example here) by allowing two arguments to modify the same variable, though I agree it's a little awkward.
    The nice thing about it is that it means that args.covariance aligns intuitively with what we want to pass to TreeTime without any internal logic-flipping. With just --no-covariance, it would be most logical to set this to True, but we'd then have to pass it on to the refine function with something like
    covariance=!(args.no_covariance) or True, or similar. This is certainly doable and not a big deal, but in comparison I ended up liking the argparse-supported solution that's a bit more logical, though a bit more redundant. I can certainly change it though, if you think it's more confusing on the user end.

  2. I don't have any strong feelings regarding --keep-root vs integrating into --root, though as you say it lines up with TreeTime. Unless @rneher has any objections I'm happy to change this.

@rneher

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

I think we discussed --root none at some point -- at least I contemplated it. My take on --root none was that None is typically the default in argparse when no argument is given and specifying a string variant of None seemed prone to cause trouble. On the other hand, we wanted TreeTime to reroot on default. I don't really have strong feelings about the covariance/no-covariance option. At the time I was thinking this would be simply an enhancement that you would never do without, but there are cases where it doesn't behave as expected.

@tsibley

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Paired flags like both of these examples are pretty common, with one of the pair usually being a default. For all the reasons @emmahodcroft noted, I'd vote to keep --covariance/--no-covariance as-is in the PR.

Similarly, I'd vote to keep --root … vs. --keep-root. --root none seems like it might mean "unroot this tree" instead of "don't modify the existing root". @rneher's comments about string variants of None being prone to causing trouble also ring true to me.

@trvrb

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Okay. This is fine. My perceived pattern had been to have just one or the other for store_true flags. If having --covariance/--no-covariance is a common pattern then that's fine. (I had also been trying to cut down on arguments in refine as there's already a lot there, but having more clearer arguments is probably better than fewer less clear ones)

@huddlej

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

Before merging, we want to upgrade to phylo-treetime 0.5.5 and test common builds to make sure everything still works.

@rneher

rneher approved these changes Apr 23, 2019

@rneher

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Did anybody test get around to testing this? I haven't had any issues. but would be good to have a few more eyes.

@emmahodcroft

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Can we merge this?

@emmahodcroft emmahodcroft merged commit 5adc284 into master May 6, 2019

2 checks passed

Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details

@rneher rneher deleted the rooting branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.