add ohlc chart to TraceFactory #259

cldougl · 2015-07-14T22:59:30Z

add ohlc chart as 2 TraceFactory methods so
TraceFactory.create_ohlc_increasing() returns a trace of the
increasing data and TraceFactory.create_ohlc_decreasing() returns a
trace of the decreasing data. In these charts increasing and decreasing
periods are typically represented by different colours (default green &
red).
pull flatten method out of _Quiver so other charts can call it

- add ohlc chart as 2 TraceFactory methods so `TraceFactory.create_ohlc_increasing()` returns a trace of the increasing data and `TraceFactory.create_ohlc_decreasing()` returns a trace of the decreasing data. In these charts increasing and decreasing periods are typically represented by different colours (default green & red). - pull flatten method out of `_Quiver` so other charts can call it

Updates to ohlc -make tests -make default for kwargs so the default colours are red/green (typical for this chart)

cldougl · 2015-07-17T22:11:32Z

@chriddyp @theengineear @aneda @etpinard Hi guys, ohlc chart is ready for review/feedback

theengineear · 2015-07-18T07:44:56Z

@cldougl your tests are failing (likely) due to the same things affecting @aneda . Once she's merged into master, you should be able to just merge master into this branch and be good to go. Else you can (a) cherry pick the changes or (b) merge her branch as-is and then merge master later before you merge into master.

so many options :)

reviewing now!

theengineear · 2015-07-18T07:47:28Z

plotly/tests/test_core/test_tools/test_trace_factory.py

+
+class TestOHLC(TestCase):
+
+    def test_unequal_o_h_l_c_length(self):


🐄 As is well put, a foolish consistency is the hobgoblin of little minds. It'd be fine to call this:

test_unequal_ohlc_length

😸

theengineear · 2015-07-18T07:55:51Z

@cldougl imo, having two separate calls for ohlc charts seems like a lot. I'm happy to keep them separate in tools, but maybe just create a single way to make an ohlc chart using TraceFactory? I think, as a user, I'd be more likely to remember TraceFactory.create_ohlc than the two alternatives.

Just a thought, happy to be dissuaded.

theengineear · 2015-07-18T08:00:28Z

Also, unless it's really common to use op, hi, lo, cl in other languages. We usually like to error on the side of explicitness:

The Zen of Python, by Tim Peters

Beautiful is better than ugly.
**Explicit is better than implicit.**
Simple is better than complex.
Complex is better than complicated.

... etc etc (try doing import this) to check them all out.

open, high, low, close

theengineear · 2015-07-18T08:17:53Z

plotly/tools.py

-    def validate(x, y, u, v,
-                 scale=.1, density=1,
-                 arrow_scale=.3, **kwargs):
+    def validate_shared(x, y, u, v,


🐄 Just a suggestion. Would this be clearer as two functions?

def validate_positive_scalars(**kwargs): """ Validates that all values given in key/val pairs are positive. Accepts kwargs to improve Exception messages. :raises: (PlotlyError) If any value is < 0 or raises. """ for key, val in kwargs.items(): try: if val <= 0: raise PlotlyError('{} must be > 0, got {}'.format(key, val) except TypeError: raise PlotlyError('{} must be a number, got {}'.format(key, val) def validate_arrays(**kwargs): """" Validates a set of named arrays. Accepts kwargs to improve Exception messages. :raises: (PlotlyError) On failure. """" # do your checks here...

I think that after the refactorings, this validate method has gotten much simpler than the docstring suggests?

Again, just a 🐄

theengineear · 2015-07-18T08:49:26Z

@cldougl yowza! this is super clean! nice job. a couple of comments above and I'm going to hold off on dancing not literally of course until tests pass.

- combine create_ohlc_increase and create_ohlc_decrease into create_ohlc - rename variables to open, high, low, close - 2 pep 8 fixes - check messages with assertRaisesRegexp

- remove create_ohlc_increase and create_ohlc_decrease - update validate_ methods so that they can be used with multiple traces when applicable

- check that direction is ‘increasing’ or ‘decreasing’

cldougl · 2015-07-21T15:21:03Z

@theengineear @chriddyp tests are passing now, can I get another review of this

theengineear · 2015-07-21T17:47:57Z

plotly/tools.py

@@ -1670,6 +1722,130 @@ def create_streamline(x, y, u, v,
                             mode='lines', **kwargs)
        return streamline

+    @staticmethod
+    def create_ohlc(open, high, low, close, direction, **kwargs):
+


🐄 I don't believe we typically leave a space between a function's call signature and the docstring.

chriddyp · 2015-07-21T17:52:54Z

@cldougl i'll review tonight or tomorrow

theengineear · 2015-07-21T17:55:47Z

only non-blocking comments from me! the interface for creating these looks good!

💃 from me, but you should also wait for a green light from @chriddyp

cldougl · 2015-07-21T17:57:41Z

@theengineear @chriddyp sounds good! Thanks!!

- add candlestick chart and tests - TF.create_candlestick uses fill and grouped legends to create the candlestick outline and the shaded body - (also oops forgot some :rtypes: in ohlc docs)

cldougl · 2015-07-22T18:22:09Z

@chriddyp @theengineear @aneda @etpinard @jackparmer candlestick charts are ready for review too (they're pretty similar to the ohlc charts but with a shaded middle)

cldougl · 2015-07-28T13:59:09Z

Yep, that sounds good

cldougl · 2015-07-28T15:18:47Z

@theengineear re the Factories subclassing dict: the idea was to make it similar to Scatter
We can change it to subclassing from object if that makes more sense because we don't need update, fromkeys, clear, etc

theengineear · 2015-07-28T15:25:24Z

Yup. Yeah, we'd only want to subclass dict if we're using it like that. It'd be clearer to just use object. If it's not too big a change, that'd be great! 🐒

cldougl · 2015-07-28T15:26:06Z

done :)

theengineear · 2015-07-28T15:26:39Z

\o/

- subclass from `object` - separated `create_ohlc()` into `_make_increasing()` and `make_decreasing()` so the user can pass kwargs to the ‘increasing’ and ‘decreasing’ traces by defining `direction` as increasing or decreasing. This keeps it consistent with `create_candlestick` - make `_make_increasing` and `_make_decreasing` static methods for ohlc and candlestick - set `hovermode` to closest for ohlc

add examples (still need average line and subplots examples)

- remove candlestick width=4 (leave as default 2) - change ohlc default to 1 - add bar gap to candlestick layout

so that you can pass in `dates=df.index` and nothing will fail with the `datetimeindex isn’t a bool`

Finance fixes

- set ohlc width to 1 - remove duplicate names in tests

chriddyp · 2015-07-29T04:04:35Z

💃 when tests pass

- update version number - add end line to tools.py

add finance charts to FigureFactory

theengineear · 2015-07-29T11:42:44Z

💥 nice!

cldougl added 2 commits July 14, 2015 18:58

add tests and update ohlc

db377d6

Updates to ohlc -make tests -make default for kwargs so the default colours are red/green (typical for this chart)

theengineear reviewed Jul 18, 2015
View reviewed changes

cldougl added 4 commits July 20, 2015 13:37

update create_ohlc

293e518

- combine create_ohlc_increase and create_ohlc_decrease into create_ohlc - rename variables to open, high, low, close - 2 pep 8 fixes - check messages with assertRaisesRegexp

make single create_ohlc called twice

7679f5c

- remove create_ohlc_increase and create_ohlc_decrease - update validate_ methods so that they can be used with multiple traces when applicable

update direction arg in ohlc

4be32bb

- check that direction is ‘increasing’ or ‘decreasing’

pulling in neda's last commit

070b372

theengineear reviewed Jul 21, 2015
View reviewed changes

add candlestick chart

497dc04

- add candlestick chart and tests - TF.create_candlestick uses fill and grouped legends to create the candlestick outline and the shaded body - (also oops forgot some :rtypes: in ohlc docs)

cldougl and others added 17 commits July 28, 2015 14:42

update candlestick docstring

e753a67

add examples (still need average line and subplots examples)

finance chart tests

5ba6283

width update

6d91d4e

- remove candlestick width=4 (leave as default 2) - change ohlc default to 1 - add bar gap to candlestick layout

change layout from dict to Layout

2b02713

pep8 💄

2f2361a

nicer default reds and greens

c3c8672

reformatted examples that focus on pandas

609b033

whoops, one more colors update

f880a5e

more explicit dates argument check

f08b712

so that you can pass in `dates=df.index` and nothing will fail with the `datetimeindex isn’t a bool`

another missed colors commit

e3e10d2

Merge branch 'financecharts' into finance-fixes

6829986

1.8.0 - OHLC and Candlestick Charts

da022e2

missing datetime imports

aa13dc2

Merge pull request #270 from plotly/finance-fixes

92bb50c

Finance fixes

update tests after width and colour edits

c69c20d

change width of ohlc to 1

168fd17

- set ohlc width to 1 - remove duplicate names in tests

cldougl added 3 commits July 29, 2015 00:05

unhide validate_streamline

15d7f19

update finance charts to resolve conflicts

f5f7881

bump version

5e5df9e

- update version number - add end line to tools.py

cldougl added a commit that referenced this pull request Jul 29, 2015

Merge pull request #259 from plotly/financecharts

4640072

add finance charts to FigureFactory

cldougl merged commit 4640072 into master Jul 29, 2015

theengineear deleted the financecharts branch September 30, 2015 06:12


		class TestOHLC(TestCase):

		def test_unequal_o_h_l_c_length(self):

Uh oh!

add ohlc chart to TraceFactory #259

add ohlc chart to TraceFactory #259

Uh oh!

Conversation

cldougl commented Jul 14, 2015

Uh oh!

cldougl commented Jul 17, 2015

Uh oh!

theengineear commented Jul 18, 2015

Uh oh!

theengineear Jul 18, 2015

Choose a reason for hiding this comment

Uh oh!

theengineear commented Jul 18, 2015

Uh oh!

theengineear commented Jul 18, 2015

Uh oh!

theengineear Jul 18, 2015

Choose a reason for hiding this comment

Uh oh!

theengineear commented Jul 18, 2015

Uh oh!

cldougl commented Jul 21, 2015

Uh oh!

theengineear Jul 21, 2015

Choose a reason for hiding this comment

Uh oh!

chriddyp commented Jul 21, 2015

Uh oh!

theengineear commented Jul 21, 2015

Uh oh!

cldougl commented Jul 21, 2015

Uh oh!

cldougl commented Jul 22, 2015

Uh oh!

cldougl commented Jul 28, 2015

Uh oh!

cldougl commented Jul 28, 2015

Uh oh!

theengineear commented Jul 28, 2015

Uh oh!

cldougl commented Jul 28, 2015

Uh oh!

theengineear commented Jul 28, 2015

Uh oh!

chriddyp commented Jul 29, 2015

Uh oh!

theengineear commented Jul 29, 2015

Uh oh!

Uh oh!