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.

Sign up for GitHub

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

Jump to bottom

Rename pip() resp columns #207

Open
wants to merge 10 commits into
base: DEV
Choose a base branch
from
Open

Rename pip() resp columns #207

wants to merge 10 commits into from

Conversation

Aeilert
Copy link

@Aeilert Aeilert commented Feb 11, 2022

First draft

@Aeilert Aeilert requested a review from tonyfujs February 11, 2022 12:55
@Aeilert Aeilert self-assigned this Feb 11, 2022
This was linked to issues Feb 11, 2022
Recode pce to hfce #193
Open
Recode survey_year to welfare_time #194
Closed
Rename year variables in final outputs #201
Open
@Aeilert Aeilert marked this pull request as draft February 11, 2022 12:57
@Aeilert Aeilert marked this pull request as ready for review February 17, 2022 14:24
tonyfujs
tonyfujs requested changes Feb 21, 2022
View reviewed changes
Copy link
Contributor

@tonyfujs tonyfujs left a comment

Choose a reason for hiding this comment

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

Hey @Aeilert,

I've made some code update to fix the empty response and some unit tests.

Currently, some ui_functions() are failing because of the column name inconsistency. They are also pulling data from lkup tables (which have not been renamed...)

One potential solution to move forward with the renaming, and still avoid making the corresponding changes at the pipeline level, would be to rename all lookup tables in the create_lkup() function. What do you think?

@Aeilert
Copy link
Author

Aeilert commented Feb 22, 2022

Thanks @tonyfujs,

I agree that renaming in columns in the create_lkup() function is a good idea. This won't cover all cases though since some AUX datasets are read directly from disk. But still something I think we could forward with

Aleksander Eilertsen and others added 6 commits February 22, 2022 04:59
Merge branch 'dev' into rename_pip_cols
d1d61a5
Rename cols in create_lkups()
5a25ac9 
* Rename cols in create_lkups in stead of in pip()
* Add new rename_cols() function
* Use rename_cols w/ get_aux_table() to rename files read from disk
Fix unit tests
4233d06 
Note: The sub-list name is still reporting_pop.
Could potentially change this as well.
Fix unit test
946e2bb
Fix unit test
78f5180
@tonyfujs
fix unit test to handle changes in max allowed povline
d7debe0
tonyfujs
tonyfujs approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@tonyfujs tonyfujs left a comment

Choose a reason for hiding this comment

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

Looks good @Aeilert Just made a small update to a unit test that was failing. (related to separate PR on increasing max PL)

@Aeilert Aeilert mentioned this pull request Mar 16, 2022
Closed
@randrescastaneda
Copy link
Member

Task linked: CU-2me746v Rename pip() resp columns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonyfujs tonyfujs tonyfujs approved these changes

Assignees

@Aeilert Aeilert

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename year variables in final outputs Recode pce to hfce Recode survey_year to welfare_time
3 participants
@Aeilert @randrescastaneda @tonyfujs