Resolve Xrefs equivalence #36
Conversation
| Name : string | ||
| Description : string | ||
| Modifiers : string | ||
| } | ||
|
|
||
| /// Parses a given string to a DBXref | ||
| static member ofString (v : string) = |
There was a problem hiding this comment.
I do not understand why you are mixing static members and instance members here, is there a need for this?
There was a problem hiding this comment.
Instance members are only possible on an already existing object. The ofString method creates a DBXref object.
| @@ -519,6 +519,47 @@ type OboOntology = | |||
| static member getSynonyms term (onto : OboOntology) = | |||
| onto.GetSynonyms term | |||
|
|
|||
| /// <summary>Checks if the given terms are treated as equivalent in the OboOntology.</summary> | |||
| /// <remarks>Note that term1 must be part of the OboOntology while term2 must be part of another ontology.</remarks> | |||
There was a problem hiding this comment.
I think this is fine for now, but i think we should eventually either add a check here or add item accession to the terms in the ontology, e.g. ontology["termName"] should return the relevant term, so this method just needs to be this.AreTermsEquivalent(termName : string, externalTerm : OboTerm) = ....
Requiring one term being from the ontology without checking/enforcing it screams for unintended behavior IMO, but as said, fine for now
| open System | ||
|
|
||
| open ARCtrl.ISA |
There was a problem hiding this comment.
out of curiosity, where is this reference needed?
There was a problem hiding this comment.
There are IsaOntologyAnnotation functions that use this. I think it's mainly used in ISA.NET and some subsequent projects but that was more like @HLWeil 's business.
| Name = "" | ||
| Accession = this.Name | ||
| RefUri = | ||
| String.split ':' this.Name |
There was a problem hiding this comment.
Is this reliable? Will all TAN have this shape? Just curious, i don't know the answer myself
There was a problem hiding this comment.
The OBO Flat File Format section defines it this way, but with SHOULD.
I, myself, should therefore add a try with there. 😉
There was a problem hiding this comment.
Nvm., String.split does not throw if there's not the given delimiter. It just returns an array with the input string as single item:
String.split ':' "klsdklsd"
val it: string[] = [|"klsdklsd"|]Therefore, no need to do anything here, and I'm fine with this output for IDs that don't follow the format recommendations.
| testList "ReturnAllEquivalentTerms" [ | ||
| testCase "returns correct terms" <| fun _ -> | ||
| let actual = testOntology.ReturnAllEquivalentTerms(testOntology2) | ||
| Expect.equal (Seq.head actual).Id testTerm4.Id "is not equal" |
There was a problem hiding this comment.
Since you are testing a sequence here, i would test the whole sequence instead of just the head
There was a problem hiding this comment.
Then it would compare objects and they are not equal (due to reference). Or does Expecto circumvent this by checking the fields' values via reflection? 🤔
(I'm not sure but) I think not.
Gonna test it.
There was a problem hiding this comment.
There is Expect.SequenceEqual.
There was a problem hiding this comment.
That's what I'm talkin' 'bout. 😄
There was a problem hiding this comment.
Yes, but this is not what I was about.
Nevertheless, for record types there's structural equality and checks for every field. Therefore, this should be no problem. 👍🏻
|
Requests are done, ready for rereview! 😃 @kMutagene |
This PR
ofStringtoCvTermfunctionality