Substitution lazy cloning #270

AntoniusW · 2020-10-01T22:22:46Z

Removes the need for upfront cloning of Substitutions when checking unification.

- Add Substitution.unify that clones the partial input substitution on demand (i.e., when an additional variable assignment is required). - Modify NaiveGrounder and AbstractLiteralInstantiationStrategy to not clone Substitutions upfront.

codecov · 2020-10-01T22:53:39Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7788331). Click here to learn what that means.
The diff coverage is 84.61%.

@@            Coverage Diff            @@
##             master     #270   +/-   ##
=========================================
  Coverage          ?   77.44%           
  Complexity        ?     2311           
=========================================
  Files             ?      178           
  Lines             ?     7782           
  Branches          ?     1352           
=========================================
  Hits              ?     6027           
  Misses            ?     1335           
  Partials          ?      420

Impacted Files	Coverage Δ	Complexity Δ
...a/at/ac/tuwien/kr/alpha/grounder/Substitution.java	`77.77% <83.33%> (ø)`	`18.00 <2.00> (?)`
.../at/ac/tuwien/kr/alpha/grounder/NaiveGrounder.java	`86.12% <100.00%> (ø)`	`69.00 <0.00> (?)`
...ntiation/AbstractLiteralInstantiationStrategy.java	`100.00% <100.00%> (ø)`	`8.00 <0.00> (?)`
.../grounder/transformation/StratifiedEvaluation.java	`97.79% <100.00%> (ø)`	`53.00 <0.00> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7788331...af66c5a. Read the comment docs.

madmike200590

I think everything is ok from a functional point of view, i.e. the code does what it's supposed to do, however, I find it hard to read and confusing in places, see more detailed comments. I'm aware not all of those are easy to address (or even necessarily need to be addressed), but I think some discussion makes sense.

madmike200590 · 2020-10-02T15:42:22Z

src/main/java/at/ac/tuwien/kr/alpha/grounder/Substitution.java

 }
- return substitution;
+
+ boolean unifyTerms(Term termNonGround, Term termGround, Substitution partialSubstitution) {


It seems wrong (or at least really confusing) to me that we now have UnificationHelper::unifyTerms(Term, Term, Substitution) as well as Substitution::unifyTerms(Term, Term). The only difference seems to be that the one in Substitution always operates on this and modifying the substitution it's called on, while the other one operates on the given partialSubstitution. I think the version in UnificationHelper should be adapted so that the other one isn't needed.

Agreed, the Substitution::unifyTerms(Term, Term) is only ever needed in unit tests, while the new one is not tested. Will adapt the tests to run on the real thing and get rid of the other.

madmike200590 · 2020-10-02T15:48:03Z

src/main/java/at/ac/tuwien/kr/alpha/grounder/Substitution.java

+ * already is a unifier, it is returned. If the unifying substitution is an extension of the input
+ * substitution, a new substitution will be returned.
+ */
+ public static Substitution unify(Atom atom, Instance instance, Substitution substitution) {


More of a general question, and maybe out of the scope of this review... We also have classes Unifier and Unification, with Unification implementing utility functions looking very similar to some of the methods in Substitution. In order to avoid duplicating code, or writing several very similar versions, would it make sense to use the (accordingly adapted) methods from Unification here?

There is a slight difference between the two (and maybe we should adapt our naming): the one in Substitution is assuming that variables only ever occur on the left-hand side (i.e., the second parameter is always ground). The version in Unification really computes the unifier between two non-ground Atoms. Maybe we should rename the methods in Substitution to instantiate() instead of unify().

I decided to rename Substitution::unify to Substitution::specializeSubstitution as it specializes the given partial substitution to also unify the given atom and the instance.

madmike200590 · 2020-10-02T15:53:39Z

src/main/java/at/ac/tuwien/kr/alpha/grounder/Substitution.java

+ private static class UnificationHelper {
+ Substitution returnSubstitution;
+
+ Substitution unify(List<Term> termList, Instance instance, Substitution partialSubstitution) {


Note that this is a matter of personal taste, but I'm not sure I agree with the return types here..
I think it might make sense if unifyTerms returned something other than boolean, thereby getting rid of the side-effects of potentially modifying the given partialSubstitution. Instead, unifyTerms could return a Substitution that may be either a new (cloned) one, or the one passed in in case nothing changed. Alternatively, we could also use an Optional<Substitution> here, where the presence of a value would indicate that a new variable got bound. The version with Optional could also be used for Substitution::unify as an alternative to returning null.

Letting unifyTerms return a Substitution is a good idea. I would not want to return an Optional however, since substitutions are used at the core of grounding and Optional is likely slower and creates more objects.

Actually, I decided to remove unifyTerms as it was only ever used in unit tests. These unit tests have been adapted to now test the new/renamed specializeSubstitution.

AntoniusW · 2020-10-02T22:36:09Z

I think everything is ok from a functional point of view, i.e. the code does what it's supposed to do, however, I find it hard to read and confusing in places, see more detailed comments. I'm aware not all of those are easy to address (or even necessarily need to be addressed), but I think some discussion makes sense.

That is exactly the reason why I like our four-eyes policy on pull requests. It forces one to deliver better code. Thank you for the comments!

- Remove superfluous Substitution.unifyTerms. - Adapt SubstitutionTest to Substitution specialization. - Remove duplicate tests from UnifierTest.

AntoniusW · 2020-10-04T21:03:54Z

Comments have been addressed, please check if this is better now.

madmike200590

Thanks for all the improvements, this looks really good now!

AntoniusW added 2 commits October 1, 2020 23:14

Clone Substitutions only if necessary.

690d1f8

- Add Substitution.unify that clones the partial input substitution on demand (i.e., when an additional variable assignment is required). - Modify NaiveGrounder and AbstractLiteralInstantiationStrategy to not clone Substitutions upfront.

Clone Substitutions only if necessary also for StratifiedEvaluation.

ecffafe

AntoniusW requested a review from madmike200590 October 1, 2020 22:22

madmike200590 requested changes Oct 2, 2020

View reviewed changes

Better name Substitution specialization/unify.

0308839

- Remove superfluous Substitution.unifyTerms. - Adapt SubstitutionTest to Substitution specialization. - Remove duplicate tests from UnifierTest.

lorenzleutgeb added efficiency enhancement labels Oct 5, 2020

Improve comments.

af66c5a

madmike200590 approved these changes Oct 7, 2020

View reviewed changes

madmike200590 merged commit 5360c49 into master Oct 7, 2020

madmike200590 deleted the substitution_lazy_cloning branch October 7, 2020 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Substitution lazy cloning #270

Substitution lazy cloning #270

AntoniusW commented Oct 1, 2020

codecov bot commented Oct 1, 2020 •

edited

Loading

madmike200590 left a comment

madmike200590 Oct 2, 2020

AntoniusW Oct 2, 2020

madmike200590 Oct 2, 2020 •

edited

Loading

AntoniusW Oct 2, 2020

AntoniusW Oct 4, 2020 •

edited

Loading

madmike200590 Oct 2, 2020

AntoniusW Oct 2, 2020

AntoniusW Oct 4, 2020

AntoniusW commented Oct 2, 2020

AntoniusW commented Oct 4, 2020

madmike200590 left a comment

Substitution lazy cloning #270

Substitution lazy cloning #270

Conversation

AntoniusW commented Oct 1, 2020

codecov bot commented Oct 1, 2020 • edited Loading

Codecov Report

madmike200590 left a comment

Choose a reason for hiding this comment

madmike200590 Oct 2, 2020

Choose a reason for hiding this comment

AntoniusW Oct 2, 2020

Choose a reason for hiding this comment

madmike200590 Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

AntoniusW Oct 2, 2020

Choose a reason for hiding this comment

AntoniusW Oct 4, 2020 • edited Loading

Choose a reason for hiding this comment

madmike200590 Oct 2, 2020

Choose a reason for hiding this comment

AntoniusW Oct 2, 2020

Choose a reason for hiding this comment

AntoniusW Oct 4, 2020

Choose a reason for hiding this comment

AntoniusW commented Oct 2, 2020

AntoniusW commented Oct 4, 2020

madmike200590 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 1, 2020 •

edited

Loading

madmike200590 Oct 2, 2020 •

edited

Loading

AntoniusW Oct 4, 2020 •

edited

Loading