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

ot_65/ot_65/ot_65.json missing a bunch of otus #2

Open
rhr opened this issue Nov 11, 2014 · 5 comments
Open

ot_65/ot_65/ot_65.json missing a bunch of otus #2

rhr opened this issue Nov 11, 2014 · 5 comments

Comments

@rhr
Copy link
Member

rhr commented Nov 11, 2014

Looks like the range otu37 through otu52 is not included in the nexson, but are referenced by tree nodes. All other files in the repo seem fine in this regard.

@mtholder
Copy link
Member

it looks like this is a couple of problems (at least). I'm not sure how the original nexson was written this way (it seems to have been an issue from the beginning, so it could be in the merge_otu function).
and I'm not sure why this is showing up as a warning rather than an error (an error would have prevented the save from working so we would have caught this earlier).
hopefully we'll be able to pull up the original upload and recreate the behavior, but I'm afraid that I don't have time to look at it right now...

@jimallman
Copy link
Member

Are we assuming that merge_otu will close any gaps in these serial IDs? I didn't realize..

I'm planning to chase opentree bugs later this week. If I dig up an old version for testing, I'll make a note of it here.

@mtholder
Copy link
Member

no. I don't think that it guarantees that it will do that, but these OTUs are referred to by leafs in the tree - making those leaves unuseable...

@jimallman
Copy link
Member

... not included in the nexson, but are referenced by tree nodes.

Yep, not sure how I missed that. This is definitely a Bad Thing.

@mtholder
Copy link
Member

The validator was not checking that the otu fields resolved to an otu. That was a major oversight.
I have a partial fix on https://github.com/OpenTreeOfLife/peyotl/tree/phylesystem-1-issue-2 (still need to validate non-v1.2 forms of nexson).

That fix let me check the files in the repo.

This may be the only study with this problem.
ot_31 and ot_32 also have problems with nodes lacking @otu fields.

But the rest of the files seem OK with the exception of some cases of the studyYear being a string rather than integer in ot_192.json ot_57.json ot_63.json ot_66.json ot_67.json ot_68.json ot_70.json ot_77.json).

Those errors may prevent the otu-not-existing error from being reported. I'll try to fix these files soon so that I can check them more thoroughly.

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

3 participants