Experimental/safe test parameters from excel files#473
Conversation
andrewbaxter439
left a comment
andrewbaxter439
left a comment
There was a problem hiding this comment.
Have noted some things that might be worth looking at specifically!
| validateRegressors(coeffCovarianceHM2CaseMales, "reg_health_mental.xlsx", "HM2_Males_C"); | ||
|
|
There was a problem hiding this comment.
This should come out as was put in to test. Will be run automatically if safeReadExcel is implemented.
| validateRegressors(coeffCovarianceHM2CaseMales, "reg_health_mental.xlsx", "HM2_Males_C"); |
| try { | ||
| Person.DoublesVariables.valueOf(keyName); | ||
| } catch (IllegalArgumentException e) { | ||
| try { | ||
| BenefitUnit.Regressors.valueOf(keyName); | ||
| } catch (IllegalArgumentException e2) { |
There was a problem hiding this comment.
This is a hacky test - checking whether looking up both Person and BenefitUnit enums errors out. The error IllegalArgumentException isn't the best fit, perhaps needs to go deeper to get a better error in each of these cases, or do a more literal check of what enum values are present in each set?
Or leave as-is if checking valueOf and returning IllegalArgumentException is a good standard way of testing?
| String[] badValueVector = new String[] {"Dag", "Not_a_valid_value"}; | ||
| String[] goodValueVector = new String[] {"Dag", "D_Home_owner", "PovertyToNonPoverty"}; | ||
| String[] keyVector = new String[] {"REGRESSOR"}; |
There was a problem hiding this comment.
The 'good' values depend on stable enums being in Person.java (e.g. D_Home_owner). If this changes, the test will fail, though the method would still work correctly.
There was a problem hiding this comment.
Pull request overview
Adds an early “safety check” for regression Excel inputs by validating that regressor names loaded into MultiKeyCoefficientMap correspond to known model enums, aiming to fail fast with clearer diagnostics.
Changes:
- Added
Parameters.validateRegressors(...)to verify regressor keys againstPerson.DoublesVariablesandBenefitUnit.Regressors. - Added
Parameters.safeReadExcel(...)wrappers that load coefficient maps and immediately validate regressors. - Added a new unit test covering valid vs invalid regressor names.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/main/java/simpaths/data/Parameters.java |
Introduces regressor validation + “safe” Excel read wrappers and applies validation to one loaded regression map. |
src/test/java/simpaths/data/ParametersTest.java |
Adds a unit test for the new regressor validation logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| coeffCovarianceHM2CaseMales = ExcelAssistant.loadCoefficientMap(Parameters.getInputDirectory() + "reg_health_mental.xlsx", countryString + "_HM2_Males_C", 1); | ||
| coeffCovarianceHM2CaseFemales = ExcelAssistant.loadCoefficientMap(Parameters.getInputDirectory() + "reg_health_mental.xlsx", countryString + "_HM2_Females_C", 1); | ||
|
|
||
| validateRegressors(coeffCovarianceHM2CaseMales, "reg_health_mental.xlsx", "HM2_Males_C"); |
| // Check across all | ||
| for (Object regressor : regressorNames) { | ||
| if (regressor instanceof MultiKey mk) { | ||
| String keyName = mk.getKey(0).toString(); |
| // This fires if the string isn't in the Enum | ||
| throw new RuntimeException("Validation failed for " + excelFileName + " in " + sheetName + | ||
| ": Regressor '" + keyName + "' not found in Person.DoublesVariables. " + | ||
| "Check for typos in Excel or missing Enums in Person.java."); |
| import java.util.Arrays; | ||
|
|
| String[] badValueVector = new String[] {"Dag", "Not_a_valid_value"}; | ||
| String[] goodValueVector = new String[] {"Dag", "D_Home_owner", "PovertyToNonPoverty"}; | ||
| String[] keyVector = new String[] {"REGRESSOR"}; | ||
|
|
||
| MultiKeyCoefficientMap badMap = new MultiKeyCoefficientMap(keyVector, badValueVector); | ||
| for (String badValue : badValueVector) {badMap.putValue(badValue, 0);} | ||
|
|
||
| MultiKeyCoefficientMap goodMap = new MultiKeyCoefficientMap(keyVector, goodValueVector); | ||
| for (String goodValue : goodValueVector) {goodMap.putValue(goodValue, 0);} | ||
|
|
||
| assertThrows(RuntimeException.class, () -> Parameters.validateRegressors(badMap, "A map designed to contain invalid values", "Sheet1")); | ||
| assertDoesNotThrow(() -> Parameters.validateRegressors(goodMap, "A map designed to contain valid values", "Sheet1")); |
|
Looks like a really useful addition |
What
Why
Before merging
safeReadExcelwrapper should replace allExcelAssistant.loadCoefficientMapreads for all regression files?