`FemalesWithChildrenByChildAgeCSfilter` only sees the last child

[amorison]

The FemalesWithChildrenByChildAgeCSfilter filter is incorrectly implemented. The offending bit is this loop, gathering children ages:

Set<Person> childrenInBU = person.getBenefitUnit().getChildren();
int[] ages = new int[childrenInBU.size()];
if (childrenInBU != null) {
    int arrayPosition = 0;
    for (Person child : childrenInBU) {
        ages[arrayPosition] = child.getDemAge();
    }
}

arrayPosition is never incremented, meaning ages will only see one child (and if there are more than one children, this means the minimum age being seen will also be 0 rather than the age of that child).

The semantic of that filter is also slightly unclear. Looking at the implementation (and especially an older implementation that's been commented out), this seems to be intended to check that all children are in the given age range. The way it's used in experiment/SimPathsObserver.java, however, seems to need to check that any child (rather than all children) is in the given age range (for which leveraging BenefitUnit::getIndicatorChildren is probably the way to go).

Note: the check childrenInBU != null is unnecessary since BenefitUnit::getChildren never returns a null. It's also too late anyway since childrenInBU.size() called before would fail on a null.

Once the semantic is clarified, I'm happy to make a PR (I also have a performance improvement for BenefitUnit::getIndicatorChildren I can throw in).

[dav-sonn]

@justin-ven do you know which is the correct filtering condition that should be applied?

• all children are in the given age range; or
• any child is in the given age range.

[justin-ven]

The code looks like a bug - the isFiltered method looks pretty awful to me. I would replace the entire method with:
Person person = (Person) object; return Indicator.True.equals(person.getBenefitUnit().getIndicatorChildren(ageFrom, ageTo));

[amorison]

@justin-ven well, the code you're proposing would check for any child being in the given age range rather than all of them. Either way, yes, the current code is needlessly convoluted on top of being wrong, but really the question is whether this is intended to check for all children being in the age range vs any of them being in the age range.

[justin-ven]

Ah - apologies for missing the latter half of the issue. Having not encountered this before, my code assumes any child in the range. No idea why you would want all children in the range. Do the code uses provide any clearer picture? If not, suggest we go with the definition implicit in my code suggestion.

[amorison]

The way this is used is what prompted me to open this issue: it's used to produce histograms of mothers being responsible of children by age range. So it did seem more natural to me that you'd want to check for any child being in the range rather than all children being in the range. I'll go with that then.