Added GO aspect convenience methods for #203

[dustine32]

Let me know if these are in the wrong place (ontol.py) or they're just, plain wrong.

[cmungall]

I would put these in a separate static/non-OO convenience module called something like go_utils

I'd also rename get_ancestors to be something more explicit like get_isa_part_of_ancestors

The logic for any is_X method should be to follow is_a only. Remember there are part-ofs between MF and BP

[deepakunni3]

@cmungall Would get_isa_partof_closure be a better name for that method? It seems to do exactly that.
We could also add get_isa_closure along with get_isa_partof_closure

[deepakunni3]

@dustine32

If you call self.ancestors(go_term, reflexive=True) then you won't have to do all_ancestors.append(go_term) as the term itself is included in the list of ancestors returned by self.ancestors().

[dustine32]

Thanks @deepakunni3 ! I'll try that option out.

[dustine32]

Yep, it worked!

[deepakunni3]

As a note for discussion - why does calling subontology() improve speed?
It looks like subontology() is called every time this method is called. Not sure if there is some caching happening somewhere.

Ideally, the following would achieve the same result:

# for isa_closure
self.ancestors(go_term, relations=['subClassOf'], reflexive=True)

# for isa_partof_closure
self.ancestors(go_term, relations=['subClassOf', 'BFO:0000050'], reflexive=True)

but while testing, I see that the proposed method is more quick at getting ancestors

[dustine32]

Yeah, I figured this out a long time ago so it might've changed (or cache is messing me up), but specifying relations without using subontology first will calculate the filtered graph using the whole GO ontology rather than just the smaller subset of "all relations" ancestors of a node.

Like, specifiying relations requires looking at edge properties (in ontol.get_filtered_graph()) whereas not just cares that an edge is there. Does this make sense?

[deepakunni3]

Thank you @dustine32
Yes, I see the difference and where the bottleneck is 👍

[dustine32]

Cool, thanks for the feedback! I'll reorganize this into a file under utils/ and make those changes.

I do remember running into the MF-part_of-BP issue for this a while ago, causing MF terms to be erroneously identified as BPs. But I got around it by just asking if it was an MF first, though is_biological_process would still be left saying an MF's a BP. I'll try limiting this to just use the 'is a' relation.

[dustine32]

I think these changes are ready to go. I now have this in util/go_utils.py and I'm only traversing the graph through is_a relations. @cmungall @deepakunni3 Let me know if I missed anything. Thanks!

[cmungall]

Great!

Note if you call go_aspect a lot, it could be made more efficient as it does 3 ancestor queries, but we can worry about optimization later

[deepakunni3]

@dustine32 Looks good 👍
Thanks for adding these. I do see get_isa_closure() and get_isa_partof_closure() methods being used a lot in the coming days.

Feel free to merge this PR

[dustine32]

Awesome thanks!