activitysim icon indicating copy to clipboard operation
activitysim copied to clipboard

Estimation fix

Open jpn-- opened this issue 3 years ago • 1 comments
trafficstars

As implemented, the estimation example for non-mandatory tour frequency included a short cut to limit the number of unique parameters to be estimated for each person type.

This was a design feature that Ben, Jeff Doyle and I implemented to get stable estimation results out of these models, as without it the model estimation was hopelessly over-specified. We didn’t have access to any original estimation source documents that laid out the inter-parameter relationships, just a massive table of estimated parameters values where tons of them were the same number. The named-parameter design was meant to get around this conundrum in the future, as you can explicitly use the same parameter multiple places, and also have different parameters with the same current value, but maybe one day they will not have the same value.

Now, some agencies are working to estimate these models, and once the model spec is written more intelligently, the short cut is not actually desirable. This PR adds a mechanism to turn it off.

jpn-- avatar Aug 24 '22 18:08 jpn--

Coverage Status

Coverage remained the same at 0.0% when pulling 586cf454c3d4ca1a9227783389a717e958fec6bc on camsys:est-fix into 8b7737cec695c0da74bcea694617793d0104f9a4 on ActivitySim:develop.

coveralls avatar Aug 25 '22 03:08 coveralls

I'd just add that I agree with @dhensle suggestion to make the default behavior for condensing parameters false, but also that it seems like we need a more widely applicable way to constrain parameters to be equal to each other in estimation so that this functionality can be used in all models, not just in non-mandatory tour frequency, and that the user can control more explicitly which parameters to collapse rather than the default behavior of the functionality. Meanwhile we might consider completely turning off the functionality. Also if enabled, the coefficients written to the 'utility' page in the spreadsheet should be renamed like p.original_name_condensed_1 or some such so that we know that this parameter was condensed and which terms the condensed parameter is being applied to, rather than the current behavior which incorrectly identifies the parameter as one of the parameters that is being condensed and results in an incorrect utility specification reported.

jfdman avatar Dec 21 '22 19:12 jfdman

I'd just add that I agree with @dhensle suggestion to make the default behavior for condensing parameters false

Looking at this again, I also agree False is the correct default arg value. It should never be used for actual parameter estimation in "real life". This argument exists only because no one had the original parameter estimation template for these models, and we wanted to demonstrate re-estimation without re-creating the code that defines how the apparent 1000+ parameters in this model group were originally estimated.

As to @jfdman's suggestion that there should be better estimation capabilities, I agree. The current estimation mode was built to mirror daysim's capabilities to update parameters while changing literally nothing about the model structure. It can be and has been used to re-estimate components with structural changes as well, but doing that estimation requires significant effort. Easing that effort is far beyond the scope of this PR.

jpn-- avatar Dec 30 '22 21:12 jpn--