Repair CDAP estimation #1026
Repair CDAP estimation #1026
Conversation
dhensle
left a comment
dhensle
left a comment
There was a problem hiding this comment.
Fix looks good. Suggest we add more detail to the change log, and one minor typo to fix.
| will have a significant impact on simulated travel behavior outcomes, as it is only | ||
| a few parameters with a large and complex model, and the overall behavioral | ||
| characteristics of 3, 4, and 5-person households are expected to be somewhat | ||
| similar to each other in any case. |
There was a problem hiding this comment.
I believe the fix would be to actually be adding a coef_[M,N,H]_xxx and coef_[M,N,H]_xxxx and giving them the same value as coef_[M,N,H]_xxxxx since these were all combined in estimation, right?
Granted, if the model had undergone some sort of calibration, that difference would have been calibrated out and they should probably leave it.
So, I think we should supply the following guidance:
- Make the new coefficients if you have estimated CDAP but not calibrated.
- Just leave it if calibrated -- minor bug with very limited sensitivity differences.
| coef_N_88,-0.05952606764773704,-0.05952606764773704,0.87709999999999999,0 | ||
| coef_N_xxx,-0.89914972777069246,-0.89914972777069246,-0.36530000000000001,0 | ||
| coef_N_xxxx,-0.72130413279127681,-0.72130413279127681,-1.3460000000000001,0 | ||
| coef_N_xxxxx,-6.6290578146794195,-6.6290578146794195,-3.4529999999999998,0 |
There was a problem hiding this comment.
Happy to see the additional coef_xxx and coef_xxxx coefficients. All these coefficients across M,N, and H make a lot more sense now with a strong negative coefficient for everyone in a 5-person household doing the same thing.
| if all((i == "x" for i in j)): # wildcards also map to empty | ||
| interact_coef_map[(c_split[1], "")] = c | ||
| # previously, wildcards also mapped empty here, but this caused a clash | ||
| # as all wildcards would map to the same coefficient name not matter the |
There was a problem hiding this comment.
minor typo here, should be "name no matter" instead of "name not matter"
2 participants
This pull request fixes an error in Larch estimation of the CDAP model, reported in #595. There was a bug in the code, where interaction terms for 3, 4, and 5 person households adopting an all-M, all-N, or all-H patterns were all mapped to the same coefficient; the original model structure has unique parameters for each household size. [1] [2]
Testing and validation
test_cdap_modeltest to include assertions verifying the correct assignment of interaction parameters, specifically checking the presence and absence of wildcard coefficients for different model sizes.test_cdap_model_loglike.csvto reflect changes in log-likelihood values resulting from the repaired coefficient mapping logic.