Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Estimation fix #591

Merged
merged 7 commits into from
Dec 30, 2022
Merged

Estimation fix #591

merged 7 commits into from
Dec 30, 2022

Conversation

jpn--
Copy link
Member

@jpn-- jpn-- commented Aug 24, 2022

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-- jpn-- changed the base branch from develop to main August 24, 2022 20:43
@jpn-- jpn-- changed the base branch from main to develop August 24, 2022 20:43
@coveralls
Copy link

Coverage Status

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

@dhensle dhensle self-requested a review October 20, 2022 16:36
@jpn-- jpn-- added this to the Phase 7 milestone Dec 13, 2022
@@ -125,17 +125,34 @@ def unavail(model, x_ca):
def nonmand_tour_freq_model(
edb_directory="output/estimation_data_bundle/{name}/",
return_data=False,
condense_parameters=True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good and works for me, but I suggest changing condence_parameters default to False. As demonstrated by work on this from MWCOG, having this turned on and not realizing what it does can lead to significant confusion.

@jfdman
Copy link

jfdman commented Dec 21, 2022

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.

@jpn--
Copy link
Member Author

jpn-- commented Dec 30, 2022

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-- jpn-- merged commit 7cc99ef into ActivitySim:develop Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants