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

Dividends add currency col #1201

Merged
merged 4 commits into from May 20, 2016

Conversation

Projects
None yet
3 participants
@mtydykov
Contributor

mtydykov commented May 13, 2016

Adds "currency" and "type" columns to dividends dataset.

@mtydykov mtydykov force-pushed the dividends-add-currency-col branch 2 times, most recently from be2e69f to 3e47e22 May 13, 2016

@coveralls

This comment has been minimized.

coveralls commented May 13, 2016

Coverage Status

Coverage increased (+0.06%) to 81.508% when pulling 1fce4a6 on dividends-add-currency-col into e188954 on master.

@@ -204,6 +245,10 @@ def setup(self, dates):
['NaT', '2014-01-04', '2014-01-14'],
['NaT', '2014-01-04']]
amounts = [['NaN', 1, 15], ['NaN', 7, 13], ['NaN', 3, 1], ['NaN', 23]]
currency_types = [[None, "$", "EUR"], [None, "EUR", "$"],
[None, "$", "EUR"], [None, "EUR"]]

This comment has been minimized.

@kglowinski

kglowinski May 16, 2016

Contributor

Should the last list here have a 3rd element?

This comment has been minimized.

@mtydykov

mtydykov May 16, 2016

Contributor

Nope, just 2. It's because in this last case, we don't know anything until the 5th, and then we find out about an announcement in euros, and we don't get any other announcements, so that's what we know until the end of our time frame.

@@ -55,8 +55,7 @@ def setup(self, dates):
'datetime64[ns]', 'NaN'
),
NEXT_ANNOUNCEMENT: self.get_expected_next_event_dates(
dates, 'datetime64[ns]', 'NaN'
),
dates, 'datetime64[ns]', 'NaN'),

This comment has been minimized.

@kglowinski

kglowinski May 16, 2016

Contributor

kruft, I suspect? Put it back, please.

@kglowinski

This comment has been minimized.

Contributor

kglowinski commented May 16, 2016

Should we start to somehow subdivide common? I feel like the strings in there tend to only be used to one "type" of pipeline loader. But they're all placed into the same file.

@mtydykov

This comment has been minimized.

Contributor

mtydykov commented May 16, 2016

Yeah, maybe the constants can go into the specific loaders/tests where they belong. Initially I thought they should have a common place because some are used in qexec, but not all of them are actually used there, so maybe splitting them up makes sense.

@kglowinski

This comment has been minimized.

Contributor

kglowinski commented May 19, 2016

Squash WIP commit, then LGTM.

@mtydykov mtydykov force-pushed the dividends-add-currency-col branch from 933b3ca to 4834256 May 19, 2016

@coveralls

This comment has been minimized.

coveralls commented May 19, 2016

Coverage Status

Coverage decreased (-0.003%) to 81.618% when pulling 4834256 on dividends-add-currency-col into 1d16309 on master.

@mtydykov mtydykov force-pushed the dividends-add-currency-col branch from 4834256 to 3efa44d May 19, 2016

@coveralls

This comment has been minimized.

coveralls commented May 19, 2016

Coverage Status

Coverage decreased (-0.003%) to 81.618% when pulling 3efa44d on dividends-add-currency-col into 1d16309 on master.

@mtydykov mtydykov force-pushed the dividends-add-currency-col branch from 3efa44d to 751a08a May 20, 2016

@coveralls

This comment has been minimized.

coveralls commented May 20, 2016

Coverage Status

Coverage decreased (-0.003%) to 81.686% when pulling 751a08a on dividends-add-currency-col into de17803 on master.

@mtydykov mtydykov merged commit ef08572 into master May 20, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mtydykov mtydykov deleted the dividends-add-currency-col branch May 20, 2016

@davidandreoletti davidandreoletti referenced this pull request Dec 20, 2016

Open

Forex support #1285

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