#1704/#1705 weighted decision selection #1718
Conversation
ghost
requested a review
from 
...ava/com/scottlogic/datahelix/generator/core/walker/decisionbased/RowSpecTreeSolverTests.java
Outdated
Show resolved
Hide resolved
...ava/com/scottlogic/datahelix/generator/core/walker/decisionbased/RowSpecTreeSolverTests.java
Outdated
Show resolved
Hide resolved
.../com/scottlogic/datahelix/generator/core/walker/rowspec/RandomRowSpecDecisionTreeWalker.java
Outdated
Show resolved
Hide resolved
| double applicabilityOfThisOption = option.getAtomicConstraints().stream() | ||
| .mapToDouble(optionAtomicConstraint -> rootNode.getAtomicConstraints().stream() | ||
| .filter(rootAtomicConstraint -> rootAtomicConstraint.getField().equals(optionAtomicConstraint.getField())) | ||
| .filter(rootAtomicConstraint -> rootAtomicConstraint instanceof InSetConstraint) | ||
| .map(rootAtomicConstraint -> (InSetConstraint)rootAtomicConstraint) | ||
| .findFirst() | ||
| .map(matchingRootAtomicConstraint -> { | ||
| double totalWeighting = matchingRootAtomicConstraint.legalValues.stream() | ||
| .mapToDouble(InSetRecord::getWeightValueOrDefault).sum(); | ||
|
|
||
| double relevantWeighting = matchingRootAtomicConstraint.legalValues.stream() | ||
| .filter(legalValue -> optionAtomicConstraint.toFieldSpec().canCombineWithLegalValue(legalValue.getElement())) | ||
| .mapToDouble(InSetRecord::getWeightValueOrDefault).sum(); | ||
|
|
||
| return relevantWeighting / totalWeighting; | ||
| }) | ||
| .orElse(1d)) | ||
| .sum(); | ||
|
|
||
| if (applicabilityOfThisOption > 1){ | ||
| double applicabilityFraction = applicabilityOfThisOption - (int) applicabilityOfThisOption; | ||
| applicabilityOfThisOption = applicabilityFraction == 0 | ||
| ? 1 | ||
| : applicabilityFraction; | ||
| } | ||
|
|
||
| if (applicabilityOfThisOption == 0){ | ||
| return new WeightedElement<>( | ||
| Merged.contradictory(), | ||
| 1 | ||
| ); | ||
| } | ||
|
|
||
| return new WeightedElement<>( | ||
| treePruner.pruneConstraintNode(constraintNode, getFields(option)), | ||
| applicabilityOfThisOption | ||
| ); |
There was a problem hiding this comment.
This is very complicated. Possibly needs refactored into a few well named methods. Also as its complicated we might need a comment explaining what this thing is doing so someone who isn't familiar with this area can understand it.
There was a problem hiding this comment.
I've added some comments to the code and refactored some of the calculations into functions, the complexity is somewhat unavoidable i think.
| .mapToDouble(optionAtomicConstraint -> rootNode.getAtomicConstraints().stream() | ||
| .filter(rootAtomicConstraint -> rootAtomicConstraint.getField().equals(optionAtomicConstraint.getField())) | ||
| .filter(rootAtomicConstraint -> rootAtomicConstraint instanceof InSetConstraint) | ||
| .map(rootAtomicConstraint -> (InSetConstraint)rootAtomicConstraint) |
There was a problem hiding this comment.
maybe make this more modular and change this to:
mapToDouble(optionAtomicConstraint -> findMatchingInSetConstraints(optionAtomicConstraint.getField(), rootNode.getAtomicConstraints())
and then define the method:
Stream findMatchingInSetConstraints(fieldtoMatch, atomicConstraints
| double totalWeighting = getWeightOfAllLegalValues(matchingRootAtomicConstraint); | ||
| double relevantWeighting = getWeightOfAllPermittedLegalValues(matchingRootAtomicConstraint, optionAtomicConstraint); | ||
|
|
||
| return relevantWeighting / totalWeighting; |
There was a problem hiding this comment.
Maybe encapsulate this in a calculateApplicability method
| lastRowSpec = weightedRowSpec; | ||
| } | ||
|
|
||
| return lastRowSpec.element(); |
There was a problem hiding this comment.
should we protect against a nullptr here in case lastRowSpec is null (eg rowSpecCache is empty could cause this). In which case return ??? (ideally some kind of 'empty' row spec rather than null - depends what calling code assumes). Or should we throw an exception as never expect this to be called with empty rowSepcCache.
Description
When a profile contains a decision (if constraint) which refers to weighted set values, currently the generator doesn't take the weightings into account.
Changes
Additional notes
Issue
Resolves #1704
Resolves #1705