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

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

Merged
merged 1 commit into from Dec 3, 2015

Conversation

rogerkwoodley
Copy link
Contributor

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.

…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
Copy link
Collaborator

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
Per email from Brad Maggard on 2015-09-16, addfieldusingcontext() doe…
@alimanfoo alimanfoo merged commit a73b7ac into petl-developers:master Dec 3, 2015
@alimanfoo
Copy link
Collaborator

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

@rogerkwoodley
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants