geneontology / obographs

Basic and Advanced OBO Graphs: specification and reference implementation
63 stars 12 forks source link

Sort JSON serialisation of obographs #47

Closed matentzn closed 1 year ago

matentzn commented 4 years ago

Not sure this is the right place, but it would be great if you could find a way that JSON serialisations are sorted so that syntactic diff tools (like git diff) can be used effectively. See also https://github.com/INCATools/ontology-development-kit/issues/150

cmungall commented 4 years ago

This is the right place. Agreed on priority. Not sure when we can dedicate resources to this.. maybe there is a quick fix in the json serialization code?

On Mon, Aug 5, 2019 at 2:38 AM Nico Matentzoglu notifications@github.com wrote:

Not sure this is the right place, but it would be great if you could find a way that JSON serialisations are sorted so that syntactic diff tools (like git diff) can be used effectively. See also INCATools/ontology-development-kit#150 https://github.com/INCATools/ontology-development-kit/issues/150

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/geneontology/obographs/issues/47?email_source=notifications&email_token=AAAMMOKFHLKVJTUSDI36XVTQC7YIVA5CNFSM4IJJKB3KYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HDKXLLA, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAMMOJELDEPM7OEHBMGYKTQC7YIVANCNFSM4IJJKB3A .

julesjacobsen commented 4 years ago

This would be really handy. You can assign this to me if you like. It's a matter of adding the @JsonPropertyOrder annotation to the class.

@JsonPropertyOrder({"genomeAssembly", "vcf", "proband", "age", "sex", "hpoIds", "pedigree"})
matentzn commented 4 years ago

a.m.a.z.i.n.g. +100

julesjacobsen commented 4 years ago

Actually, some of this won't be possible - the order of Axioms coming out of the OWLAPI cannot be guaranteed as they use a HashSet internally and a HashMap both of which is inherently unordered. This is incredibly irritating. If only they used a Linked implementation or a Guava Immutable version both of which are ordered.

matentzn commented 4 years ago

Right.. Somehow the owl api serialisers are sorting the axioms though; maybe we can reuse the code for the functional syntax serialiser! That should put it all in order?

julesjacobsen commented 4 years ago

If you fancy having a dig around in the OWLAPI code you might be able to figure out a way to do this. The relevant entry point is OWLOntology.getAxioms()

matentzn commented 4 years ago

Given this, can you load the axioms in a list and sort them prior to processing @julesjacobsen ?

julesjacobsen commented 4 years ago

Thanks for checking, this should be simple enough!

julesjacobsen commented 4 years ago

Can you also give me a preferred order for node, edge etc. fields?

matentzn commented 4 years ago

This you need to check with @cmungall I dont know anything about the obographs format at all unfortunately..

julesjacobsen commented 4 years ago

Just to tantalise you I appear to have a fully reproducible OWL -> obographs -> json outputs the same every time.

But it in the immutable branch Nico, could you see if that works for you? It's a complete re-write of the core so needs a thorough test.

matentzn commented 4 years ago

uuuuuu I would love to jules, but unfortunately I am unable to prioritise this with the madness of simon leaving at the moment.. I can offer code review! But testing would just postpone this feature for a very long time.

julesjacobsen commented 4 years ago

Damn. Code review would be good, but there is hardly any code to review! The point of the change was to replace all the manual builder code with auto-generated code.

It ought to be a more or less drop-in replacement for the existing library. Looking at Robot there should be no code changes required, but you might need to get your Jackson dependencies all nicely lined-up as these can throw errors otherwise.

matentzn commented 4 years ago

If you share with me a jar, and an example invocation of a conversion, I will try to schedule a testing session with all ontologies I work with next week..

julesjacobsen commented 4 years ago

That would be awesome - do you want a new version number or the current 0.2.1?

matentzn commented 4 years ago

I dont really mind, I usually only access the code through ROBOT.. a generic obographs.jar will do, I will delete it when the tests are done!

julesjacobsen commented 4 years ago

Much appreciated- just change the extension to .jar obographs-1.0.0 .zip

matentzn commented 4 years ago

Great, if you give me an example invocation that reads an OWL file and writes a JSON (and vice versa), I will take care of it! :P

julesjacobsen commented 4 years ago
File file = // your ontology file here
// usual OWLAPI invocations:
OWLOntologyManager m = OWLManager.createOWLOntologyManager();
OWLOntology ontology = m.loadOntologyFromOntologyDocument(file);

// OboGraphs
FromOwl fromOwl = new FromOwl();
GraphDocument gd = fromOwl.generateGraphDocument(ontology);

Path fn = file.toPath().getFileName();
String jsonStr = OgJsonGenerator.render(gd);
String ofn = fn.toString().replace(".obo", ".json").replace(".owl", ".json");
File jsonOutFile = new File(ofn);
FileUtils.writeStringToFile(jsonOutFile, jsonStr);

// read it back in from JSON - this is where you might find an error
GraphDocument graphDocument = OgJsonReader.readFile(jsonOutFile);

// ideally, but not tried this yet!
assertThat(gd, equalTo(graphDocument))
matentzn commented 4 years ago

Thats great, thanks! I will do it in the next 5-10 working days.

julesjacobsen commented 4 years ago

Tried that last bit and did indeed find an error - please use this version! obographs-1.0.0.zip

I really appreciate you taking the time to do this. Mind you, in the end everyone should benefit as the API is more useable, much more robust. For example it has fewer null values and does other useful things like implement hashCode(), equals(), toString() which are all needed for correctly functioning classes.

matentzn commented 4 years ago

@julesjacobsen could you send me a working version of the pom.xml you are using? I keep getting dependency issues.. I tried to simply copy the dependencies from the obographs project, but even then I get this:

Exception in thread "main" java.lang.NoSuchMethodError: com.google.common.collect.ImmutableSet.toImmutableSet()Ljava/util/stream/Collector;
    at org.geneontology.obographs.owlapi.FromOwl.generateGraph(FromOwl.java:233)
    at org.geneontology.obographs.owlapi.FromOwl.generateGraphDocument(FromOwl.java:64)
    at OBOGraphsTesterApp.run(OBOGraphsTesterApp.java:35)
    at OBOGraphsTesterApp.<init>(OBOGraphsTesterApp.java:24)
    at OBOGraphsTesterApp.main(OBOGraphsTesterApp.java:75)

Thanks!

julesjacobsen commented 4 years ago

@matentzn The source code is here: https://github.com/julesjacobsen/obographs/tree/immutables

These are the updated dependencies: https://github.com/julesjacobsen/obographs/blob/6d8cc2f8c522b5571c0b96d9623c04f69fcad424/pom.xml#L198-L213

julesjacobsen commented 4 years ago

Actually one more thought - this needs to be backwards compatible with json produced using the old version. Can you use the updated version from master (this has the assertion order fixed) and do a diff of the JSON produced by both FromOwl tests. i.e. (OWL - FromOwl -> OgJsonGenerator.render() -> JSON ) for both 1.0.0 and a new build from https://github.com/geneontology/obographs/tree/master. Make sense?

matentzn commented 4 years ago

Works with the new dependencies! Unfortunately, I cant right now do anything else; I am just too swamped. Would it help you if I send you a zip of test files?

julesjacobsen commented 4 years ago

Now even more sorted! Sure, if you're swamped, send me a zip of test files. obographs-1.0.0.zip

julesjacobsen commented 4 years ago

This should be fixed by https://github.com/geneontology/obographs/pull/63

julesjacobsen commented 4 years ago

It's working- 3ddcee2e37f03a4886ae94738f9084f2472c9508

@matentzn should be testing this in Robot. n.b. Updating Robot to use the new Immutables branch will require a code change - see https://github.com/ontodev/robot/issues/670

cmungall commented 1 year ago

Closing at looks like this is done