search

INCATools / whelk-rs

Whelk is an OWL EL reasoner
MIT License
11 stars 4 forks source link

Consider using horned-owl's IRI type instead of strings for entity identifiers #12

Open balhoff opened 1 year ago

balhoff commented 1 year ago

All the entity types in whelk have an ID that is represented by a string: https://github.com/INCATools/whelk-rs/blob/74e76cf4ddf35617b12d48389d3963445781bd3c/src/whelk/model.rs#L69-L77

In OWL this ID is an IRI. We should think about making this more explicit and directly using IRIs for the IDs. Would this save any memory? (When we translate from OWL to the Whelk model we could pass the IRI directly). Some recent IRI discussion in horned-owl: https://github.com/phillord/horned-owl/issues/56

We should probably wait for any IRI changes in horned-owl to be resolved before taking any action.

phillord commented 1 year ago

Nice idea. I think it confirms to me that we should not be validating IRIs by default, but only on demand.

It might save some memory but it should also simplify your code a bit -- I think you could ditch all the Rc's that you have currently in model.rs, as IRI already takes care of that (generically). You also gain multi-thread with Arc.

I guess this raises the question of code organisation. For whelk, it makes no difference, but I wonder whether IRI belongs in it's own crate.

phillord commented 1 year ago

Slightly different question, but I'd quite like to have whelk-rs as a dependency of horned-bin, so I can add a horned-reason command in. Currently that would give us a circular dependency, though. We should have a talk at some point about how to organise this!

balhoff commented 1 year ago

We're depending on horned-bin so that we can use the parsers (in tests and also in our minimal CLI, which could be a separate product).

phillord commented 1 year ago

Just the convenience parse_path method as far as I can see. I think there is something to be reworked here, but guess I can leave it will we come to right a horned-reason command.

balhoff commented 1 year ago

Just the convenience parse_path method as far as I can see. I think there is something to be reworked here, but guess I can leave it will we come to right a horned-reason command.

Oh I see, for our simple CLI we can probably just use the parsers directly if that lets us drop the horned-bin dependency.