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

Per email from Brad Maggard on 2015-09-16, addfieldusingcontext() doe… #343

Merged
merged 1 commit into from Dec 3, 2015

Conversation

Projects
None yet
2 participants
@rogerkwoodley
Contributor

rogerkwoodley commented Sep 16, 2015

Per email from Brad Maggard on 2015-09-16, addfieldusingcontext() doesn't have access to the new field, making calculations like running sum more complicated.

The generator yields the tuple from the original table. It now creates a new Record() object with the additional field and the previous row's value, providing access to the prior row's new field.

I profiled both time and memory usage - the extra step of creating the Record() instance is minimal.

On a random table of 1MM rows, it added 100KB of memory at runtime and changed execution time from 63 seconds to 65 seconds on my test.

Per email from Brad Maggard on 2015-09-16, addfieldusingcontext() doe…
…sn't have access to the new field, making calculations like running sum more complicated.

The generator yields the tuple from the original table.  It now creates a new Record() object with the additional field and the previous row's value, providing access to the prior row's new field.
@alimanfoo

This comment has been minimized.

Collaborator

alimanfoo commented Sep 17, 2015

Thanks a lot Roger, I'll review asap.

On Wednesday, September 16, 2015, Roger Woodley notifications@github.com
wrote:

Per email from Brad Maggard on 2015-09-16, addfieldusingcontext() doesn't
have access to the new field, making calculations like running sum more
complicated.

The generator yields the tuple from the original table. It now creates a
new Record() object with the additional field and the previous row's value,
providing access to the prior row's new field.

I profiled both time and memory usage - the extra step of creating the
Record() instance is minimal.

On a random table of 1MM rows, it added 100KB of memory at runtime and

changed execution time from 63 seconds to 65 seconds on my test.

You can view, comment on, or merge this pull request online at:

#343
Commit Summary

  • Per email from Brad Maggard on 2015-09-16, addfieldusingcontext()
    doesn't have access to the new field, making calculations like running sum
    more complicated.

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#343.

Alistair Miles
Head of Epidemiological Informatics
Centre for Genomics and Global Health http://cggh.org
The Wellcome Trust Centre for Human Genetics
Roosevelt Drive
Oxford
OX3 7BN
United Kingdom
Web: http://purl.org/net/aliman
Email: alimanfoo@googlemail.com alimanfoo@gmail.com
Tel: +44 (0)1865 287721

@alimanfoo alimanfoo added this to the v1.1 milestone Nov 25, 2015

alimanfoo added a commit that referenced this pull request Dec 3, 2015

Merge pull request #343 from rogerkwoodley/develop
Per email from Brad Maggard on 2015-09-16, addfieldusingcontext() doe…

@alimanfoo alimanfoo merged commit a73b7ac into petl-developers:master Dec 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alimanfoo

This comment has been minimized.

Collaborator

alimanfoo commented Dec 3, 2015

Thank you Roger, sorry it took so long to get to this.

@rogerkwoodley

This comment has been minimized.

Contributor

rogerkwoodley commented Dec 3, 2015

Oh, no need to apologize - you have your regular life, managing releases, etc. to worry about.

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