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

BUG: IchimokuKinkoHyo techinical factor has wrong default inputs #1638

Merged
merged 1 commit into from Jan 6, 2017

Conversation

@luca-s
Copy link
Contributor

@luca-s luca-s commented Jan 6, 2017

Very small bug fix: creating a IchimokuKinkoHyo factor raises an exception because the default inputs are not in line with the factor's documentation

@coveralls
Copy link

@coveralls coveralls commented Jan 6, 2017

Coverage Status

Coverage remained the same at 87.216% when pulling 2765538 on luca-s:IchimokuKinkoHyo_fix into 8ef2279 on quantopian:master.

@@ -598,7 +598,7 @@ class IchimokuKinkoHyo(CustomFactor):
'kijun_sen_length': 26,
'chikou_span_length': 26,
}
inputs = USEquityPricing.high, USEquityPricing.close
inputs = [USEquityPricing.high, USEquityPricing.low, USEquityPricing.close]

This comment has been minimized.

@richafrank

richafrank Jan 6, 2017
Member

Thanks @luca-s , makes sense to me! Could I ask for two improvements?

  1. Would you make this a tuple of inputs instead of a list, so that no one can mistakenly modify this class-level attribute?
  2. Would you add a simple test method to tests.pipeline.test_technical.IchimokuKinkoHyoTestCase which ensures that this factor doesn't blow up on computing with the default inputs, as you were seeing?

This comment has been minimized.

@luca-s

luca-s Jan 6, 2017
Author Contributor

I fixed "1" but it's not straightforward to add a test for "2", no other factor has that test.

This comment has been minimized.

@luca-s

luca-s Jan 6, 2017
Author Contributor

Just to be clear, as I was a little confusing: IchimokuKinkoHyo should have 3 default inputs but they added only 2, it was simply an oversight. I don't believe we need a test for this

This comment has been minimized.

@richafrank

richafrank Jan 6, 2017
Member

Ok, sounds fine. Thanks for the update!

@luca-s luca-s force-pushed the luca-s:IchimokuKinkoHyo_fix branch from 2765538 to 126d4aa Jan 6, 2017
@coveralls
Copy link

@coveralls coveralls commented Jan 6, 2017

Coverage Status

Coverage remained the same at 87.216% when pulling 126d4aa on luca-s:IchimokuKinkoHyo_fix into 8ef2279 on quantopian:master.

@richafrank richafrank merged commit 2259297 into quantopian:master Jan 6, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.