Skip to content

Commit

Permalink
fix: Recommended Fit API needs to ignore any unassigned entities/valu…
Browse files Browse the repository at this point in the history
…es (#618)

With nullable vars, the solution may contain many unassigned entities/values.
But the user only wants recommendations for the one that they specified.
Therefore we need to ignore every other one.

I also marginally improve composite filters, deduplicating and
decomposing where possible.

Closes #581.
  • Loading branch information
triceo committed Feb 7, 2024
1 parent d2ce741 commit 964adbd
Show file tree
Hide file tree
Showing 24 changed files with 404 additions and 132 deletions.
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package ai.timefold.solver.core.impl.constructionheuristic.placer;

import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionFilter;
import ai.timefold.solver.core.impl.phase.event.PhaseLifecycleListener;

public interface EntityPlacer<Solution_> extends Iterable<Placement<Solution_>>, PhaseLifecycleListener<Solution_> {

EntityPlacer<Solution_> rebuildWithFilter(SelectionFilter<Solution_, Object> filter);

}
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
import java.util.Iterator;

import ai.timefold.solver.core.impl.heuristic.move.Move;
import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionFilter;
import ai.timefold.solver.core.impl.heuristic.selector.common.iterator.UpcomingSelectionIterator;
import ai.timefold.solver.core.impl.heuristic.selector.move.MoveSelector;
import ai.timefold.solver.core.impl.heuristic.selector.move.decorator.FilteringMoveSelector;

public class PooledEntityPlacer<Solution_> extends AbstractEntityPlacer<Solution_> implements EntityPlacer<Solution_> {

Expand All @@ -20,6 +22,11 @@ public Iterator<Placement<Solution_>> iterator() {
return new PooledEntityPlacingIterator();
}

@Override
public EntityPlacer<Solution_> rebuildWithFilter(SelectionFilter<Solution_, Object> filter) {
return new PooledEntityPlacer<>(FilteringMoveSelector.of(moveSelector, filter::accept));
}

private class PooledEntityPlacingIterator extends UpcomingSelectionIterator<Placement<Solution_>> {

private PooledEntityPlacingIterator() {
Expand All @@ -31,7 +38,7 @@ protected Placement<Solution_> createUpcomingSelection() {
if (!moveIterator.hasNext()) {
return noUpcomingSelection();
}
return new Placement<Solution_>(moveIterator);
return new Placement<>(moveIterator);
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
import java.util.List;

import ai.timefold.solver.core.impl.heuristic.move.Move;
import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionFilter;
import ai.timefold.solver.core.impl.heuristic.selector.common.iterator.UpcomingSelectionIterator;
import ai.timefold.solver.core.impl.heuristic.selector.entity.EntitySelector;
import ai.timefold.solver.core.impl.heuristic.selector.entity.decorator.FilteringEntitySelector;
import ai.timefold.solver.core.impl.heuristic.selector.move.MoveSelector;

public class QueuedEntityPlacer<Solution_> extends AbstractEntityPlacer<Solution_> implements EntityPlacer<Solution_> {
Expand All @@ -28,6 +30,11 @@ public Iterator<Placement<Solution_>> iterator() {
return new QueuedEntityPlacingIterator(entitySelector.iterator());
}

@Override
public EntityPlacer<Solution_> rebuildWithFilter(SelectionFilter<Solution_, Object> filter) {
return new QueuedEntityPlacer<>(FilteringEntitySelector.of(entitySelector, filter), moveSelectorList);
}

private class QueuedEntityPlacingIterator extends UpcomingSelectionIterator<Placement<Solution_>> {

private final Iterator<Object> entityIterator;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
import java.util.Collections;
import java.util.Iterator;

import ai.timefold.solver.core.impl.heuristic.move.Move;
import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionFilter;
import ai.timefold.solver.core.impl.heuristic.selector.common.iterator.UpcomingSelectionIterator;
import ai.timefold.solver.core.impl.heuristic.selector.move.MoveSelector;
import ai.timefold.solver.core.impl.heuristic.selector.value.EntityIndependentValueSelector;
import ai.timefold.solver.core.impl.heuristic.selector.value.decorator.EntityIndependentFilteringValueSelector;
import ai.timefold.solver.core.impl.heuristic.selector.value.decorator.FilteringValueSelector;

public class QueuedValuePlacer<Solution_> extends AbstractEntityPlacer<Solution_> implements EntityPlacer<Solution_> {

Expand Down Expand Up @@ -44,7 +46,7 @@ protected Placement<Solution_> createUpcomingSelection() {
}
}
valueIterator.next();
Iterator<Move<Solution_>> moveIterator = moveSelector.iterator();
var moveIterator = moveSelector.iterator();
// Because the valueSelector is entity independent, there is always a move if there's still an entity
if (!moveIterator.hasNext()) {
return noUpcomingSelection();
Expand All @@ -54,4 +56,11 @@ protected Placement<Solution_> createUpcomingSelection() {

}

@Override
public EntityPlacer<Solution_> rebuildWithFilter(SelectionFilter<Solution_, Object> filter) {
return new QueuedValuePlacer<>(
(EntityIndependentFilteringValueSelector<Solution_>) FilteringValueSelector.of(valueSelector, filter),
moveSelector);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,22 @@

import java.util.Arrays;

import ai.timefold.solver.core.api.domain.solution.PlanningSolution;
import ai.timefold.solver.core.api.score.director.ScoreDirector;

/**
* Combines several {@link SelectionFilter}s into one.
* Does a logical AND over the accept status of its filters.
*
* @param <Solution_> the solution type, the class with the {@link PlanningSolution} annotation
* @param <T> the selection type
*/
final class CompositeSelectionFilter<Solution_, T> implements SelectionFilter<Solution_, T> {
record CompositeSelectionFilter<Solution_, T>(SelectionFilter<Solution_, T>[] selectionFilterArray)
implements
SelectionFilter<Solution_, T> {

static final SelectionFilter NOOP = (scoreDirector, selection) -> true;

final SelectionFilter<Solution_, T>[] selectionFilterArray;

CompositeSelectionFilter(SelectionFilter<Solution_, T>[] selectionFilterArray) {
this.selectionFilterArray = selectionFilterArray;
}

@Override
public boolean accept(ScoreDirector<Solution_> scoreDirector, T selection) {
for (SelectionFilter<Solution_, T> selectionFilter : selectionFilterArray) {
for (var selectionFilter : selectionFilterArray) {
if (!selectionFilter.accept(scoreDirector, selection)) {
return false;
}
Expand All @@ -34,17 +27,18 @@ public boolean accept(ScoreDirector<Solution_> scoreDirector, T selection) {

@Override
public boolean equals(Object other) {
if (this == other)
return true;
if (other == null || getClass() != other.getClass())
return false;
CompositeSelectionFilter<?, ?> that = (CompositeSelectionFilter<?, ?>) other;
return Arrays.equals(selectionFilterArray, that.selectionFilterArray);
return other instanceof CompositeSelectionFilter<?, ?> otherCompositeSelectionFilter
&& Arrays.equals(selectionFilterArray, otherCompositeSelectionFilter.selectionFilterArray);
}

@Override
public int hashCode() {
return Arrays.hashCode(selectionFilterArray);
}

@Override
public String toString() {
return "CompositeSelectionFilter[" + Arrays.toString(selectionFilterArray) + ']';
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,29 @@ static <Solution_, T> SelectionFilter<Solution_, T> compose(SelectionFilter<Solu
* As defined by {@link #compose(SelectionFilter[])}.
*/
static <Solution_, T> SelectionFilter<Solution_, T> compose(List<SelectionFilter<Solution_, T>> filterList) {
var distinctFilterArray = filterList.stream()
.flatMap(filter -> {
if (filter == CompositeSelectionFilter.NOOP) {
return Stream.empty();
} else if (filter instanceof CompositeSelectionFilter<Solution_, T> compositeSelectionFilter) {
// Decompose composites if necessary; avoids needless recursion.
return Arrays.stream(compositeSelectionFilter.selectionFilterArray);
} else {
return Stream.of(filter);
}
})
.distinct()
.toArray(SelectionFilter[]::new);
return switch (distinctFilterArray.length) {
return switch (filterList.size()) {
case 0 -> CompositeSelectionFilter.NOOP;
case 1 -> distinctFilterArray[0];
default -> new CompositeSelectionFilter<>(distinctFilterArray);
case 1 -> filterList.get(0);
default -> {
var distinctFilterArray = filterList.stream()
.flatMap(filter -> {
if (filter == CompositeSelectionFilter.NOOP) {
return Stream.empty();
} else if (filter instanceof CompositeSelectionFilter<Solution_, T> compositeSelectionFilter) {
// Decompose composites if necessary; avoids needless recursion.
return Arrays.stream(compositeSelectionFilter.selectionFilterArray());
} else {
return Stream.of(filter);
}
})
.distinct()
.toArray(SelectionFilter[]::new);
yield switch (distinctFilterArray.length) {
case 0 -> CompositeSelectionFilter.NOOP;
case 1 -> distinctFilterArray[0];
default -> new CompositeSelectionFilter<>(distinctFilterArray);
};
}
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ private EntitySelector<Solution_> applyFiltering(EntitySelector<Solution_> entit
}
// Do not filter out initialized entities here for CH and ES, because they can be partially initialized
// Instead, ValueSelectorConfig.applyReinitializeVariableFiltering() does that.
entitySelector = new FilteringEntitySelector<>(entitySelector, filterList);
entitySelector = FilteringEntitySelector.of(entitySelector, SelectionFilter.compose(filterList));
}
return entitySelector;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package ai.timefold.solver.core.impl.heuristic.selector.entity.decorator;

import java.util.Iterator;
import java.util.List;
import java.util.ListIterator;
import java.util.Objects;

Expand All @@ -18,20 +17,24 @@ public final class FilteringEntitySelector<Solution_>
extends AbstractDemandEnabledSelector<Solution_>
implements EntitySelector<Solution_> {

public static <Solution_> FilteringEntitySelector<Solution_> of(EntitySelector<Solution_> childEntitySelector,
SelectionFilter<Solution_, Object> filter) {
if (childEntitySelector instanceof FilteringEntitySelector<Solution_> filteringEntitySelector) {
return new FilteringEntitySelector<>(filteringEntitySelector.childEntitySelector,
SelectionFilter.compose(filteringEntitySelector.selectionFilter, filter));
}
return new FilteringEntitySelector<>(childEntitySelector, filter);
}

private final EntitySelector<Solution_> childEntitySelector;
private final SelectionFilter<Solution_, Object> selectionFilter;
private final boolean bailOutEnabled;

private ScoreDirector<Solution_> scoreDirector = null;

public FilteringEntitySelector(EntitySelector<Solution_> childEntitySelector,
List<SelectionFilter<Solution_, Object>> filterList) {
this.childEntitySelector = childEntitySelector;
if (filterList == null || filterList.isEmpty()) {
throw new IllegalArgumentException(
getClass().getSimpleName() + " must have at least one filter, but got (" + filterList + ").");
}
this.selectionFilter = SelectionFilter.compose(filterList);
private FilteringEntitySelector(EntitySelector<Solution_> childEntitySelector, SelectionFilter<Solution_, Object> filter) {
this.childEntitySelector = Objects.requireNonNull(childEntitySelector);
this.selectionFilter = Objects.requireNonNull(filter);
bailOutEnabled = childEntitySelector.isNeverEnding();
phaseLifecycleSupport.addEventListener(childEntitySelector);
}
Expand Down Expand Up @@ -172,12 +175,8 @@ private long determineBailOutSize() {

@Override
public boolean equals(Object other) {
if (this == other)
return true;
if (other == null || getClass() != other.getClass())
return false;
FilteringEntitySelector<?> that = (FilteringEntitySelector<?>) other;
return Objects.equals(childEntitySelector, that.childEntitySelector)
return other instanceof FilteringEntitySelector<?> that
&& Objects.equals(childEntitySelector, that.childEntitySelector)
&& Objects.equals(selectionFilter, that.selectionFilter);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,9 @@ private MoveSelector<Solution_> applyFiltering(MoveSelector<Solution_> moveSelec
ConfigUtils.newInstance(config, "filterClass", config.getFilterClass());
SelectionFilter<Solution_, Move<Solution_>> finalFilter =
baseFilter == null ? selectionFilter : SelectionFilter.compose(baseFilter, selectionFilter);
return new FilteringMoveSelector<>(moveSelector, finalFilter);
return FilteringMoveSelector.of(moveSelector, finalFilter);
} else if (baseFilter != null) {
return new FilteringMoveSelector<>(moveSelector, baseFilter);
return FilteringMoveSelector.of(moveSelector, baseFilter);
} else {
return moveSelector;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,22 @@

public final class FilteringMoveSelector<Solution_> extends AbstractMoveSelector<Solution_> {

public static <Solution_> FilteringMoveSelector<Solution_> of(MoveSelector<Solution_> moveSelector,
SelectionFilter<Solution_, Move<Solution_>> filter) {
if (moveSelector instanceof FilteringMoveSelector<Solution_> filteringMoveSelector) {
return new FilteringMoveSelector<>(filteringMoveSelector.childMoveSelector,
SelectionFilter.compose(filteringMoveSelector.filter, filter));
}
return new FilteringMoveSelector<>(moveSelector, filter);
}

private final MoveSelector<Solution_> childMoveSelector;
private final SelectionFilter<Solution_, Move<Solution_>> filter;
private final boolean bailOutEnabled;

private ScoreDirector<Solution_> scoreDirector = null;

public FilteringMoveSelector(MoveSelector<Solution_> childMoveSelector,
private FilteringMoveSelector(MoveSelector<Solution_> childMoveSelector,
SelectionFilter<Solution_, Move<Solution_>> filter) {
this.childMoveSelector = childMoveSelector;
this.filter = filter;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public static <Solution_> EntityIndependentValueSelector<Solution_> filterPinned
// Don't incur the overhead of filtering values if there is no pinning support.
return sourceValueSelector;
}
return (EntityIndependentValueSelector<Solution_>) FilteringValueSelector.create(sourceValueSelector,
return (EntityIndependentValueSelector<Solution_>) FilteringValueSelector.of(sourceValueSelector,
(scoreDirector, selection) -> {
var entity = inverseVariableSupply.getInverseSingleton(selection);
if (entity == null) { // Unassigned.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ protected ValueSelector<Solution_> applyFiltering(ValueSelector<Solution_> value
if (variableDescriptor.hasMovableChainedTrailingValueFilter()) {
filterList.add(variableDescriptor.getMovableChainedTrailingValueFilter());
}
valueSelector = FilteringValueSelector.create(valueSelector, filterList);
valueSelector = FilteringValueSelector.of(valueSelector, SelectionFilter.compose(filterList));
}
return valueSelector;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package ai.timefold.solver.core.impl.heuristic.selector.value.decorator;

import java.util.Iterator;
import java.util.List;

import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionFilter;
import ai.timefold.solver.core.impl.heuristic.selector.value.EntityIndependentValueSelector;
Expand All @@ -10,9 +9,9 @@ public final class EntityIndependentFilteringValueSelector<Solution_>
extends FilteringValueSelector<Solution_>
implements EntityIndependentValueSelector<Solution_> {

public EntityIndependentFilteringValueSelector(EntityIndependentValueSelector<Solution_> childValueSelector,
List<SelectionFilter<Solution_, Object>> filterList) {
super(childValueSelector, filterList);
EntityIndependentFilteringValueSelector(EntityIndependentValueSelector<Solution_> childValueSelector,
SelectionFilter<Solution_, Object> filter) {
super(childValueSelector, filter);
}

@Override
Expand All @@ -26,7 +25,7 @@ public Iterator<Object> iterator() {
determineBailOutSize());
}

protected long determineBailOutSize() {
private long determineBailOutSize() {
if (!bailOutEnabled) {
return -1L;
}
Expand Down
Loading

0 comments on commit 964adbd

Please sign in to comment.