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

specify the columns to get from pums #35

Merged
merged 8 commits into from Feb 3, 2017

Conversation

Projects
None yet
3 participants
@Eh2406
Contributor

Eh2406 commented Jan 19, 2017

This massively reduced the memory use of our recipe.

looks good to go, thoughts?

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Jan 20, 2017

Contributor

test fixed

Contributor

Eh2406 commented Jan 20, 2017

test fixed

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 20, 2017

Coverage Status

Coverage decreased (-0.1%) to 81.342% when pulling 04e3de2 on SEMCOG:usecols into 554788a on UDST:master.

coveralls commented Jan 20, 2017

Coverage Status

Coverage decreased (-0.1%) to 81.342% when pulling 04e3de2 on SEMCOG:usecols into 554788a on UDST:master.

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Jan 23, 2017

Contributor

@janowicz thoughts? Do you want me to squash the commits? Do you know why testing starter2 works locally but not on travis?

Contributor

Eh2406 commented Jan 23, 2017

@janowicz thoughts? Do you want me to squash the commits? Do you know why testing starter2 works locally but not on travis?

@janowicz

This comment has been minimized.

Show comment
Hide comment
@janowicz

janowicz Jan 24, 2017

Contributor

Thanks @Eh2406! Happy to hear about the reduced memory use. I'll get a chance to review this and look into those questions later in the week

Contributor

janowicz commented Jan 24, 2017

Thanks @Eh2406! Happy to hear about the reduced memory use. I'll get a chance to review this and look into those questions later in the week

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Feb 2, 2017

Contributor

Did you have a chance to look at this?

Contributor

Eh2406 commented Feb 2, 2017

Did you have a chance to look at this?

@janowicz

This comment has been minimized.

Show comment
Hide comment
@janowicz

janowicz Feb 3, 2017

Contributor

This is a nice change 👍 . Thanks @Eh2406. I observed lower memory usage here too.
One thing users should note is that if they're relying on a particular PUMS variable in the output that is not in h_pums_cols / p_pums_cols, then they should add that variable to the *_pums_cols tuple (or join it to the output).

The starter2 recipe runs locally for me too. I think backing out the starter2 test is fine- it can take awhile to run and is just an alternative recipe example. The main starter is more appropriate for a test.

Contributor

janowicz commented Feb 3, 2017

This is a nice change 👍 . Thanks @Eh2406. I observed lower memory usage here too.
One thing users should note is that if they're relying on a particular PUMS variable in the output that is not in h_pums_cols / p_pums_cols, then they should add that variable to the *_pums_cols tuple (or join it to the output).

The starter2 recipe runs locally for me too. I think backing out the starter2 test is fine- it can take awhile to run and is just an alternative recipe example. The main starter is more appropriate for a test.

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Feb 3, 2017

Contributor

Do you want me to squash the commits?
How can we make the use of *_pums_cols clearer?
What else should be done before merge?

Contributor

Eh2406 commented Feb 3, 2017

Do you want me to squash the commits?
How can we make the use of *_pums_cols clearer?
What else should be done before merge?

@janowicz

This comment has been minimized.

Show comment
Hide comment
@janowicz

janowicz Feb 3, 2017

Contributor

No need to squash. I added a comment about *_pums_cols. I think its ready to merge, thanks!

Contributor

janowicz commented Feb 3, 2017

No need to squash. I added a comment about *_pums_cols. I think its ready to merge, thanks!

@janowicz

This comment has been minimized.

Show comment
Hide comment
@janowicz

janowicz Feb 3, 2017

Contributor

The failed travis tests are related to #36 which will be resolved separately.

Contributor

janowicz commented Feb 3, 2017

The failed travis tests are related to #36 which will be resolved separately.

@janowicz janowicz merged commit bf3d5c8 into UDST:master Feb 3, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment