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

New example - custom axis tickformatter #3314

Merged
merged 4 commits into from
Dec 12, 2015

Conversation

lbrito1
Copy link
Contributor

@lbrito1 lbrito1 commented Dec 10, 2015

issues: fixes #3193

By defining a custom TickFormatter model, we are able to remove the weekend/holiday gaps in the candlestick financial chart. Dates are formatted into an array of Strings, e.g. ['Sep 01', 'Sep 02', ...], which is passed to the custom ticker as a property and then rendered on the axis.

The downside to this approach is that we lose the dynamic nature of Datetimei. This can be overcome by making DateGapTickFormatter receive more than one list of Strings, each one representing a different Date format, e.g. List 1 = ['Sep 01', 'Sep 02', ...]; List 2 = ['Sep', 'Sep', ... 'Oct', ...] and so on. This remains as a suggestion only and is not implemented by this commit.

By defining a custom TickFormatter model, we are able to remove the weekend/holiday gaps in the candlestick financial chart. Dates are formatted into an array of Strings, e.g. ['Sep 01', 'Sep 02', ...], which is passed to the custom ticker as a property and then rendered on the axis.

The downside to this approach is that we lose the dynamic nature of Datetimei. This can be overcome by making DateGapTickFormatter receive more than one list of Strings, each one representing a different Date format, e.g. List 1 = ['Sep 01', 'Sep 02', ...]; List 2 = ['Sep', 'Sep', ... 'Oct', ...] and so on. This remains as a suggestion only and is not implemented by this commit.
@bryevdv
Copy link
Member

bryevdv commented Dec 10, 2015

@lbrito1 quote note, our automated image diff testing machinery assumes the output filename matches the python filename, e.g. foo.py -> foo.html I think if you make that change the automated tests will pass.

@lbrito1
Copy link
Contributor Author

lbrito1 commented Dec 10, 2015

@bryevdv Thanks, fixed that.

"""

df = pd.DataFrame(MSFT)[:50]
df = df.set_index([range(0,len(df))])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should wrap range(...) in list() for python 3, or maybe use six.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup this is causing the one remaining test failure: https://travis-ci.org/bokeh/bokeh/jobs/96149634#L1032

@bryevdv
Copy link
Member

bryevdv commented Dec 11, 2015

One other suggestion I think some of the description here should go in either a comment at the top of the example or in comments spread throughout.

Refactored date_labels definition using list comprehension instead of map + lambda.

Removed df index setting. The pandas df imported from bokeh.sampledata.stocks.MSFT has integer indices, so we don't need to define them manually with df.set_index. This should be done only if the df doesn't have an integer index.

Also added some comments spread out through the example.
@lbrito1
Copy link
Contributor Author

lbrito1 commented Dec 11, 2015

Thanks for the suggestions @bryevdv . I realized the MSFT df used in the example actually has integer indices by default, so df.set_index([range(0,len(df))]) is not necessary. I removed that line and added a note that in case df has an index of a type other than integer, say datetime, it should be set as an integer sequence.

I also changed the lambda map to list comprehension and added some comments throughout the example. Not sure what the commenting policy is here (if any), so if the comments are subpar in any way let me know.

@bryevdv
Copy link
Member

bryevdv commented Dec 12, 2015

LGTM @bokeh/core any final thoughts? I will merge tomorrow if not.

# Strings (e.g. ['Sep 01', 'Sep 02', ...]) passed to the date_labels property.
class DateGapTickFormatter(TickFormatter):
date_labels = List(item_type=String)
__implementation__ = """
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Indentation is off, should be 4 spaces.
  2. For readability, one empty line before __implementation__ should be added.
  3. item_type is unnecessary, List(String) is the proper style (which should resemble scala's List[String] syntax).

@mattpap
Copy link
Contributor

mattpap commented Dec 12, 2015

I pointed out some minor stylistic complaints. Otherwise LGTM.

@mattpap mattpap added this to the 0.11.0 milestone Dec 12, 2015
Fixed indentation and improved coffeescript for loop using one-liner syntax
@lbrito1
Copy link
Contributor Author

lbrito1 commented Dec 12, 2015

@mattpap Nice, I wasn't aware of Coffeescript's one-line loop syntax. I updated the branch with your suggestions.

@bryevdv
Copy link
Member

bryevdv commented Dec 12, 2015

Thanks for the great PR @lbrito1 !

bryevdv added a commit that referenced this pull request Dec 12, 2015
New example - custom axis tickformatter
@bryevdv bryevdv merged commit 5f098e3 into bokeh:master Dec 12, 2015
@damianavila damianavila removed this from the 0.11.0 milestone Dec 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

desired_num_ticks not working when using custom x_range
4 participants