Newest gocamgen fixes #532
Conversation
Full regen of WB, MGI models with fixes from biolink/ontobio#532
| self.line.append(entity) | ||
|
|
||
| def __str__(self): | ||
| return f"Header: {','.join(self.header)} - Line: {','.join(self.line)}" |
There was a problem hiding this comment.
I find "string {} to format".format(foo) clearer, and you can name your arguments too: "string {foo} to format".format(foo=foo)
| @@ -13,19 +11,39 @@ | |||
| IPI_ECO_CODE = "ECO:0000353" | |||
|
|
|||
|
|
|||
| class GoAssocWithFrom: | |||
There was a problem hiding this comment.
Take a look at dataclasses like are used in the association.py module (where GoAssociation comes from). They're nice to write and read. Don't worry about fixing, but just something to look out for in the future.
There was a problem hiding this comment.
Also, curious about what this class does/is for!
| @@ -19,11 +19,11 @@ | |||
| # GO:0003674-2 enabled_by WB:WBGene00001173-2 | |||
| # Are there any "-"s in identifiers? | |||
|
|
|||
|
|
|||
| class AnnotationSubgraph(MultiDiGraph): | |||
There was a problem hiding this comment.
Just a design note, should AnnotationSubgraph actually be a MultiDiGraph? Like, are you going to place it in an exact context in which a MultiDiGraph will be used, taking advantage of the shared functionality (the fact that both classes have the same methods)? Or do clients/users/callers of AnnotationSubgraph actually need to know about the specific behaviors that are specialized for AnnotationSubGraph and the fact that you're using a MultiDiGraph is sort of an incendental implementation detail (for the caller)? Is AnnotationSubGraph just inheriting from MultiDiGraph out of convenience to obtain functionality of that class?
One way you can tell is if your added methods (here: add_instance_of_class) are not called by any overridden methods (here: add_edges). Do your added methods provide more interface that happen to rely on the fact that you have access to a MultiDiGraph? If your class has an instance of a MultiDiGraph as member variable, could you rewrite add_instance_of_class in terms of a member MultiDiGraph instead without any loss? Can you write your own sense of add_edge that specifies your extra relation more conretely, just forwarding what you need to an instance of MultiDiGraph?
I think in this case maybe so? This is where the "favor composition over inheritance" OOP rule comes in. Composition ends up being more flexible than inheriting generally. Inheritance is useful in only very specific situations. See: https://stackoverflow.com/questions/49002/prefer-composition-over-inheritance
Just food for thought! You don't have to change anything for your PR!
There was a problem hiding this comment.
Great point! I think initially I had intended for a much more colorful class in AnnotationSubgraph, but I can just refactor to just keep a MultiDiGraph member variable and interact with it as needed.
| @@ -37,7 +37,9 @@ def add_edge(self, u_for_edge, relation, v_for_edge, key=None, **attr): | |||
| # {'sparql_var': 'GO_0045944_1'} | |||
| # Returns the networkx graph node ID used in tracking instances | |||
| # Use this node ID to get instance IRI (once it's defined) and SPARQL variable name | |||
There was a problem hiding this comment.
also just a note, these comments documenting the behavior of your method could be turned into doc comments inside the def of the method with the triple quotes:
"""
Doc String
"""
collapsed_assoc:GoAssociation.evidence.with_support_fromfor with/from dataGoAssociationproperties DS from Annotation properties should allow multiple values of the same key #513negatedattributedatenow in ISO format (as opposed to "2021-3-2")