SmartDataAnalytics / DL-Learner

A tool for supervised Machine Learning in OWL and Description Logics
http://dl-learner.org
GNU General Public License v3.0
152 stars 34 forks source link

ObjectMinCardinality in ClosedWorldReasoner #71

Closed aris-github closed 5 years ago

aris-github commented 5 years ago

Dear all,

I think the following code is buggy for 2 reasons:

https://github.com/SmartDataAnalytics/DL-Learner/blob/d7e2cc70af16b553afef50ce6a58f0271435b304/components-core/src/main/java/org/dllearner/reasoning/ClosedWorldReasoner.java#L1075-L1117

  1. this code fails to take into account the "exact cardinality" part of the minCardinality. line 1102 adds the entry to the return set, but within the for loop. So if the Collection<OWLIndividual> inds has n suitable individuals, and the min cardinality is n, then in the n-th loop, the variable nrOfFillers is increased to n, but since the for loop will terminate after n-th loop, the code will miss the result

  2. line 1106 shall be changed to something like if (inds.size() - index < number - nrOfFillers) {. The current version does not consider the individuals that are already positive for the filler.

Altogether, I would suggest the following correction:

else if (description instanceof OWLObjectMinCardinality) {
            OWLObjectPropertyExpression property = ((OWLObjectMinCardinality) description).getProperty();
            OWLClassExpression filler = ((OWLObjectMinCardinality) description).getFiller();

            // get instances of filler concept
            SortedSet<OWLIndividual> targetSet = getIndividualsImpl(filler);

            // the mapping of instances related by r
            Map<OWLIndividual, ? extends Collection<OWLIndividual>> mapping = getTargetIndividuals(property);

            SortedSet<OWLIndividual> returnSet = new TreeSet<>();

            int number = ((OWLObjectMinCardinality) description).getCardinality();

            for (Entry<OWLIndividual, ? extends Collection<OWLIndividual>> entry : mapping.entrySet()) {
                int nrOfFillers = 0;
                int index = 0;
                Collection<OWLIndividual> inds = entry.getValue();

                // we do not need to run tests if there are not sufficiently many fillers
                if (inds.size() < number) {
                    continue;
                }

                for (OWLIndividual ind : inds) {
                    // stop inner loop when nr of fillers is reached
                    if (nrOfFillers >= number) {
                        break;
                    }
                    // early abort when too many instance checks failed, i.e. there are not enough remaining candidates
                    if (inds.size() - index < number - nrOfFillers) {
                        break;
                    }

                    if (targetSet.contains(ind)) {
                        nrOfFillers++;
                    }
                    index++;
                }

                if(nrOfFillers >= number) {
                    returnSet.add(entry.getKey());
                }                   
            }
            return returnSet;
        }
SimonBin commented 5 years ago

thank you very much for your thorough analysis

SimonBin commented 5 years ago

Applied in 5f32582f7