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

Full transition model looses the index name and saves orca table with more than local columns #35

Open
cvanoli opened this issue Dec 22, 2020 · 2 comments
Assignees

Comments

@cvanoli
Copy link
Contributor

cvanoli commented Dec 22, 2020

I would like to improve two things in the utils.full_transition model.

  1. It happens that the new dataframe that results from this operation:
new, added_hh_idx, new_linked = model.transition(
        hh, year, linked_tables=linked_tables)

which is then saved as the new orca table, looses the index name, for example 'household_id'
So the solution I propose is to add this line:
new.index.name = hh.index.name
right after this line, to make sure the index name gets preserved.

  1. The agents table is called with the local columns and the settings.get('add_columns', []):
    hh = agents.to_frame(agents.local_columns + settings.get('add_columns', [])).
    But then, when the new table is saved as an orca table, orca.add_table(agents.name, new), all the columns specified in the to_frame() are passed as local_columns which is a change that the model should not do. So I propose that when registering the new data frame as an orca table, we pass the local_columns as well, like this:
    orca.add_table(agents.name, new[agents.local_columns])
@smmaurer
Copy link
Member

These both sound like great improvements to me!

As part of the update, we should probably check whether there's other code anywhere that assumes the old behavior, in case we need to change anything in those places. One approach could be to find a model that uses utils.full_transition(), set a random seed, and run the whole model once with the old function and once with the new function. If the output is identical, then there probably aren't any side effects from the change.

@cvanoli
Copy link
Contributor Author

cvanoli commented Jan 4, 2021

Okay, I'll do that. I'm thinking that I can include this fix in the linked_tables_addin branch, since that branch is also a fix for the transition model, except that adds something to the simple_transition model. The issue is #36.

So with that branch, we can test the simple and full transition models.

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

No branches or pull requests

3 participants