Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

57 loader refactor #70

Merged
merged 9 commits into from Mar 13, 2017

Conversation

Projects
None yet
3 participants
Contributor

julesjacobsen commented Mar 13, 2017

Added a bunch of code/deprecated OWLLoader towards #57

Code is hopefully self-documenting, using a fluid API. For example, in the KnowldegBaseModule:

    //The OwlKnowledgeBase.Loader uses the ELKReasonerFactory and Concurrency.CONCURRENT as defaults. 
    this.bmKnowledgeBase = OwlKnowledgeBase.loader()
        .loadOntologies(ontologyUris) 
        .loadDataFromOntologies(ontologyDataUris)
        .loadDataFromTsv(dataTsvs) 
        .loadCuries(curies)
        .createKnowledgeBase(); 

    logger.info("Created BMKnowledgebase"); 

There is one possible issue though - the server didn't start. I'm not sure if I screwed up some Guice magic or not, but getting rid of the injector.getAllBindings() logger statement allowed the server to start and superficially appears to work.

julesjacobsen added some commits Mar 8, 2017

@julesjacobsen julesjacobsen Bumped curie-util version to 0.0.2. 1373735
@julesjacobsen julesjacobsen Updated BMKnowledgeBaseOWLAPIImpl to use java.util.Optional as now pr…
…oduced from CurieUtil.

Removed useless right-hand class types from collection initialisation. e.g. = new ArrayList<String>() -> new ArrayList<>();
efa7a6b
@julesjacobsen julesjacobsen Updated pom to use Guava 21 as this has support for Java 8 streams. b25e8a3
@julesjacobsen julesjacobsen Merge branch 'master' of https://github.com/monarch-initiative/owlsim-v3
 into 57-loader_refactor

# Conflicts:
#	owlsim-core/src/main/java/org/monarchinitiative/owlsim/kb/impl/BMKnowledgeBaseOWLAPIImpl.java
28c993c
@julesjacobsen julesjacobsen Changed Optional from Guava to Java.util version. 2b14614
@julesjacobsen julesjacobsen Added back missing code from bungled merge. 1a34b04
@julesjacobsen julesjacobsen Issue #57 - Added new OwlKnowledgeBase class as a main entry point fo…
…r creating a BMKnowledgeBaseOWLAPIImpl - @deprecated OWLLoader.

Added new supporting classes Ontology and OntologyDataSource, OntologyLoadException to help the OwlKnowledgeBase do its thing.
OWLLoader is only partially removed from the tests at present.
Removed overly verbose logging from FilterEngine.
Added commented-out clean-up of KnowledgeBaseModule - server still fails to start with ot without this.
b3c3422
@julesjacobsen julesjacobsen Issue #57 - Fixed server startup issue. Cleaned-up KnowledgeBaseModule. bd6ea08

@julesjacobsen julesjacobsen requested review from cmungall and jnguyenx Mar 13, 2017

@cmungall

Looks good

Some of the name class names are a bit confusing, but maybe I'm just lacking familiarity with some java idioms.

OwlKnowledgeBase is abstract but has no subclasses? It seems to be a holder for a loader. It's not really a knowledge base itself?

I think we can do any renaming after a merge if required, the structure here seems great.

Contributor

julesjacobsen commented Mar 13, 2017 edited

Umm, yeah. Not sure why I made it abstract exactly. I think I wanted to add the OwlKnowledgeBase functionality directly to the BMKnowledgeBase interface. I could reduce its visibility so the only way to make a BMKnowledgeBase would be off the interface itself like this:

BMKnowledgeBase knowledgebase = BMKnowledgeBase.owlLoader()...createKnowledgeBase();

Would this make a bit more sense?

As for the OwlKnowledgeBase not being a knowledge base - I called it that because being uninstantiable you could only call OwlKnowledgeBase.loader(). I though this read better than OwlKnowledgeBaseLoader.loader() which suffered from too many 'loaders' in the name.

@jnguyenx jnguyenx merged commit 84ce6cb into monarch-initiative:master Mar 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@julesjacobsen julesjacobsen deleted the julesjacobsen:57-loader_refactor branch Mar 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment