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

Set "Myr" as ^ot:branchLengthTimeUnit for trees whose ot:branchLengthMode is set to 'Time' #10

Open
jimallman opened this issue Jun 2, 2016 · 6 comments

Comments

@jimallman
Copy link
Member

jimallman commented Jun 2, 2016

Due to a minor UI bug (now fixed), this property was not being set in the web-app UI, and in many cases it will be empty. We might want to sweep the phylesystem repo for any trees whose ot:branchLengthMode is set to ot:time, and (if this unit is still empty) assert "Myr" instead.

N.B. As of today (June 2, 2016) "Myr" has been the only valid choice for this property, but there will soon be other units available. So this change should only be done for older trees, if at all.

@jar398
Copy link
Member

jar398 commented Oct 1, 2016

I believe there are only two studies fitting this description: pg_1766 and pg_2870. They can be fixed in the curator app.

for f in `find study -name "*.json"`; do if grep -q '"^ot:branchLengthMode": "ot:time",' $f; then if ! grep -q Myr $f; then echo $f; fi; fi; done
study/ot_68/ot_768/ot_768.json
study/pg_66/pg_1766/pg_1766.json
study/pg_70/pg_2870/pg_2870.json

@jar398
Copy link
Member

jar398 commented Oct 1, 2016

I checked these two studies. The trees in question appear not to have branch lengths at all, or else all branches have the same length. At least that's how it looks in the tree viewer. So I'm inclined to say that having the mode be ot:time is wrong in both cases. Not sure how best to fix, but setting the unit to Myr is almost certainly wrong. Anyone have suggestions?

@snacktavish
Copy link
Member

It looks like, may be due to this original bug, we have many studies which have "^ot:branchLengthTimeUnit": "Myr", even though the branch lengths are not in terms of time, e.g. "^ot:branchLengthMode": "ot:changesCount",

https://github.com/OpenTreeOfLife/phylesystem-1/search?q=%22%5Eot%3AbranchLengthMode%22%3A+%22ot%3AchangesCount%22%2C++%22%5Eot%3AbranchLengthTimeUnit%22%3A+%22Myr%22

@snacktavish
Copy link
Member

The most recent example I can find is: https://github.com/OpenTreeOfLife/phylesystem-1/blob/master/study/ot_29/ot_729/ot_729.json
Which suggests this may have been fixed in 2016... but maybe was actually in issue in the treebase import? I'm having trouble digging up the treebase nexml directly... I think it should be here: http://purl.org/phylo/treebase/phylows/study/TB2:S18336?format=nexml but it isn't

@jimallman
Copy link
Member Author

It looks like, may be due to this original bug, we have many studies which have "^ot:branchLengthTimeUnit": "Myr", even though the branch lengths are not in terms of time, e.g. "^ot:branchLengthMode": "ot:changesCount",

This doesn't quite fit the history of fixes, since the original bug was just about having empty values for the expected time unit. It's possible that this is happening if someone chooses a time-based mode, then chooses changesCount or whatever. I'll check to see if the UI is smart enough to clear the time-units field in that case.

Or, we reason that the time-units value is moot (ignored) if the branch-length mode is not time-based..?

@snacktavish
Copy link
Member

Yep - it looks like if you click on time, and the then go back, it gets set!

OpenTreeOfLife/phylesystem-0@92de900

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

No branches or pull requests

3 participants