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

Added record() functionality to accept dynamic names #355

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@CaptainKanuk
Contributor

CaptainKanuk commented Jun 27, 2014

In response to https://github.com/quantopian/qexec/issues/3142

Added functionality for record to accept positional args of the form (str1, value1, str2, value2, ... ) before the keyword arguments. This enables the user to record values with names defined by dynamic strings rather than hardcoding.

This PR is a precursor to a qexec PR which enables full functionality of this feature through the API. I'll submit the qexec PR as soon as this PR has been merged.

@CaptainKanuk

This comment has been minimized.

Contributor

CaptainKanuk commented Jun 27, 2014

@ssanderson and @richafrank If you could take a look at this as well that would be great, thanks. I can show you my qexec code as well, I don't know how to safely submit a PR for that before this zipline PR has been merged. Scott mentioned I could, but I didn't have time to have him show me.
Here is my branch:
https://github.com/quantopian/qexec/tree/Record-with-dynamic-naming

@coveralls

This comment has been minimized.

coveralls commented Jun 27, 2014

Coverage Status

Coverage decreased (-0.05%) when pulling 4174841 on record-dynamic-naming into 4c9cf13 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 27, 2014

Coverage Status

Coverage decreased (-0.05%) when pulling 2d1be44 on record-dynamic-naming into 4c9cf13 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 28, 2014

Coverage Status

Coverage increased (+0.01%) when pulling 86a0553 on record-dynamic-naming into 4c9cf13 on master.

@CaptainKanuk

This comment has been minimized.

Contributor

CaptainKanuk commented Jun 30, 2014

I actually think I want zip because I'd rather have it break on a mismatched set of arguments that continue executing and get an unhelpful error later in the code due to a "None" value. The six library is helpful though and I've already used it for iteritems.

@@ -563,6 +565,52 @@ def test_algo_record_vars(self):

np.testing.assert_array_equal(incr, range(1, 201))

def test_algo_record_positional(self):

This comment has been minimized.

@ssanderson

ssanderson Jun 30, 2014

Member

I realize this is probably copy/pasted from other tests, but is there a good reason for running the full 252 days here? That will end up being a decent-length test, and it seems like overkill for this functionality. Maybe consider moving this up into TestRecordAlgorithm which only runs 4 bars, which should be plenty for testing this functionality.

This comment has been minimized.

@CaptainKanuk

CaptainKanuk Jun 30, 2014

Contributor

Fixed


np.testing.assert_array_equal(incr, range(1, 201))

def test_algo_record_mixture(self):

This comment has been minimized.

@ssanderson

ssanderson Jun 30, 2014

Member

Same comment here as above.

"""
Track and record local variable (i.e. attributes) each day.
"""
for name, value in kwargs.items():
args = [iter(args)] * 2

This comment has been minimized.

@ssanderson

ssanderson Jun 30, 2014

Member

There's a little bit of magic happening here: You create two copies of an iterator, then create a list of pairs by zipping the same iterator together. It'd be easy to read this and think that it duplicates the values (i.e. [(a0, a0), (a1, a1), ...]) as opposed to iterating in pairs (i.e. [(a0, a1), (a2, a3), ...], so a comment explaining what's happening and why might be helpful.

This comment has been minimized.

@CaptainKanuk

CaptainKanuk Jun 30, 2014

Contributor

Added comments.

@CaptainKanuk CaptainKanuk assigned ehebert and unassigned jbredeche Jun 30, 2014

for name, value in kwargs.items():
# Make 2 objects both referencing the same iterator
args = [iter(args)] * 2
# Zip now takes one from L1 and one from L2 and returns a tuple

This comment has been minimized.

@ssanderson

ssanderson Jun 30, 2014

Member

Not sure what L1 and L2 are referring to here, otherwise 👍 for this comment.

@CaptainKanuk CaptainKanuk assigned ssanderson and unassigned ehebert Jun 30, 2014

ENH: Added dynamic name functionality to record() API function.
Added the ability to pass *args before the **kwargs so that positional
arguments of the form name, value can be recorded.
@coveralls

This comment has been minimized.

coveralls commented Jun 30, 2014

Coverage Status

Coverage increased (+0.02%) when pulling f377e2e on record-dynamic-naming into f8e84ec on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 30, 2014

Coverage Status

Coverage increased (+0.02%) when pulling d790da6 on record-dynamic-naming into f8e84ec on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 30, 2014

Coverage Status

Coverage increased (+0.02%) when pulling d790da6 on record-dynamic-naming into f8e84ec on master.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Jul 2, 2014

@CaptainKanuk

This comment has been minimized.

Contributor

CaptainKanuk commented Jul 2, 2014

I think it was merged, I don't know why the pull request is still open. I'm gonna double check with @ssanderson about the github dark magic.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Jul 2, 2014

Probably was just merged locally.

@twiecki twiecki closed this Jul 2, 2014

@CaptainKanuk

This comment has been minimized.

Contributor

CaptainKanuk commented Jul 2, 2014

I'll add a note to the release doc.

@freddiev4 freddiev4 deleted the record-dynamic-naming branch Sep 6, 2018

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