Feature/159 add longitudinal employment calculations#160
Feature/159 add longitudinal employment calculations#160andrewbaxter439 merged 22 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds longitudinal employment calculations by introducing new methods, filters, and tests to compute proportions of individuals shifting employment status between periods.
- Adds lagged employment filters and calculations to derive the proportions moving into and out of employment.
- Updates tests and the simulation collector to utilize these new statistics.
- Introduces a new EmploymentStatistics entity and related filtering functionality.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/simpaths/data/statistics/EmploymentStatisticsTest.java | Tests to validate proportions of employment status changes using new methods. |
| src/test/java/simpaths/data/filters/EmploymentHistoryFilterTest.java | Tests for filtering by prior employment history; contains potential spelling issues. |
| src/main/java/simpaths/model/Person.java | Added methods to retrieve lagged employment status. |
| src/main/java/simpaths/experiment/SimPathsCollector.java | Updated to persist and schedule employment statistics alongside other outputs. |
| src/main/java/simpaths/data/statistics/EmploymentStatistics.java | New entity that calculates and updates employment statistics using the new filters. |
| src/main/java/simpaths/data/filters/EmploymentHistoryFilter.java | Implements the filter for employment history based on lagged employment values. |
src/test/java/simpaths/data/filters/EmploymentHistoryFilterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/simpaths/data/filters/EmploymentHistoryFilterTest.java
Outdated
Show resolved
Hide resolved
…t.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…t.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…59-add-longitudinal-employment-calculations
igelstorm
left a comment
There was a problem hiding this comment.
Just added some thoughts about testing, but I appreciate none of the analogous existing code has tests anyway, and I don't want to hold this PR up for the sake of it! Let me know what you think - some of my concerns could maybe be addressed separately for the sake of not holding the actual validation work up.
| assertEquals(0.5, isNotEmpToEmp.getDoubleValue(IDoubleSource.Variables.Default)); | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
I may be missing something, but am I right in saying these tests aren't actually using the EmploymentStatistics class? They're just duplicating the logic inside it. If so, changing that class wouldn't be able to break these tests, so I'm not sure they're really doing what they claim to do?
There was a problem hiding this comment.
Good point. Calculating statistics right now needs a model object passed to the EmploymentStatistics.update() model, so also a bit stuck till we can mock that.
Inclined to leave these parts in as placeholders, but can add a comment that these tests aren't sufficient and should be replaced when mock models are available?
There was a problem hiding this comment.
Yes, that might be better than nothing if we're sure we will get to it?
Actually I think implementing this with a mock might be quite straightforward (and a good demonstration example), and I could have a go at this today. Happy to do either as a separate PR or add to this branch, whatever you prefer? (Doing it as part of this PR might have lower admin overhead, but maybe not if you're in a hurry to merge...)
There was a problem hiding this comment.
yeah want to try it as part of this?
There was a problem hiding this comment.
OK, it's much further from trivial than I thought - I think I'll have to do it as a separate PR if that's okay!
In order to actually test EmploymentStatistics itself, we need to figure out how to create test Persons that are more true-to-life than the really lightweight ones that we're currently creating (with the Person(boolean regressionModel) constructor) - for example, it seems like they need to have a BenefitUnit associated with them. This is definitely a problem worth trying to solve, but I think it's beyond the scope of this PR.
There was a problem hiding this comment.
If useful, this is why I created a test BenefitUnit class (to mimic the regression model Person one) and managed to get it working in the UC mental health branch:
This was the minimal setup I needed to get a linked Person/BenefitUnit/Household to run a test, ignoring most of the baggage of all three
This reverts commit 52b25a1.
igelstorm
left a comment
There was a problem hiding this comment.
Fine from my perspective - might still want input on whether the calculations are actually correct since I've focused on the implementation and testing!
|
Hi @dkopasker - last sense-check here. These lines should be:
It passes tests to show that it's giving expected results within these calculations. If those are the correct ways to understand |
What
Why
To check