Skip to content
New issue

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

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix #643: class names shortened and number of tests trimmed a bit #746

Merged
merged 1 commit into from
Feb 1, 2017

Conversation

abrokenjester
Copy link
Contributor

This PR addresses GitHub issue: #643 .

Briefly describe the changes proposed in this PR:

  • ForwardChainingSchemaCachingRDFSInferencer renamed to SchemaCachingRDFSInferencer: both shorter and more accurate
  • removed inactive testsuite for combination of spin and new inferencer

Signed-off-by: Jeen Broekstra <jeen.broekstra@gmail.com>
@abrokenjester
Copy link
Contributor Author

@hmottestad I know you had a preference to have the "ForwardChaining..." bit in there, but there are other/better ways to inform our users about this new reasoner, and it was also not quite accurate (given that 'forward chaining' is something that applies to rule-based inferencers, and this new reasoner is not, strictly speaking, rule-based).

Copy link
Contributor

@ansell ansell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

import org.junit.Rule;
import org.junit.rules.TemporaryFolder;

public class NativeStoreInferencingTestForwardChainingSchemaCachingRDFSInferencer extends InferencingTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would never have been run as its classname doesn't end in Test or TestCase

@hmottestad
Copy link
Contributor

Forward chaining is accurate. It first forward chaines the tbox into lookup tables, and then uses that to forward chain inferences on each inserted triple.

The difference between this forward chaining and the one done by a rule engine is that I cache parts of the "chain" so that that part doesn't need to forward chained again each time data is added.

Also, with forward chaining all the inferred triples are generated at insertion time. Which is the case for this reasoner.

@hmottestad
Copy link
Contributor

Anyhow, it is a very long and unwieldy name.

@hmottestad
Copy link
Contributor

hmottestad commented Feb 1, 2017

Can you change it to "RDFSInferencer".

The fact that is uses schema caching for optimisation doesn't necessarily need to be a part of the name. And if we add more things to the reasoner to make it faster or more concurrent, then we don't have to change the name again.

@hmottestad
Copy link
Contributor

or RDFSReasoner

@abrokenjester
Copy link
Contributor Author

abrokenjester commented Feb 1, 2017

The problem with that is that it doesn't distinguish it sufficiently from the existing (forward chaining) inferencer.

I'm also out of time - release is today and I need to get this merged. I suspect that once we have things stable and tested "in the field" a bit more, we'll do a refactor of this stuff anyway, hopefully we can fit that in for the next major release.

@abrokenjester abrokenjester merged commit 3367028 into master Feb 1, 2017
@abrokenjester abrokenjester deleted the issues/#643-inferencer-name branch February 1, 2017 23:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants