-
Notifications
You must be signed in to change notification settings - Fork 251
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
Add Geom.candlestick #1215
Add Geom.candlestick #1215
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1215 +/- ##
==========================================
+ Coverage 90.31% 90.75% +0.44%
==========================================
Files 39 39
Lines 4242 4512 +270
==========================================
+ Hits 3831 4095 +264
- Misses 411 417 +6
Continue to review full report at Codecov.
|
47494cc
to
69b9a51
Compare
I tried to plot a dataframe with 20 rows, https://gist.github.com/iblis17/7fc5a709eb201f8bbc6a7b6faa4479c4 |
the x-axis is a date, and it's skipping the weekends |
is it able to remove the weekends in the |
will do, but I need to figure out my TODO (inverse color) first. |
src/geom/boxplot.jl
Outdated
BoxplotGeometry(Gadfly.Stat.boxplot(method=method), suppress_outliers, tag) | ||
function BoxplotGeometry(; method=:tukey, suppress_outliers=false, suppress_fences=false, | ||
candlestick=false, tag=empty_tag) | ||
stat = candlestick ? Gadfly.Stat.identity() : Gadfly.Stat.boxplot(method=method) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the use of candlestick
as a Bool
here. For a better example of the preferred style, see the Gadfly polygon.jl file, and Geom.ellipse
. Note there is no ellipse::Bool
within struct PolygonGeometry
.
69b9a51
to
c808a03
Compare
What kind of API design is suitable for this case: |
Here is the Gadfly way. The idea is to write an # scales and aes are function arguments, which I simulate here
# You can use aes.open and aes.close, rather than the hinges which I use here
scales = Dict(:color=>Scale.color_discrete_manual("red","blue"))
aes = Gadfly.Aesthetics()
aes.upper_hinge = rand(10)
aes.lower_hinge = rand(10)
# The apply_statistic function:
# function apply_statistic(stat::CandlestickStatistic, scales, coord, aes)
hlgroup = aes.upper_hinge .> aes.lower_hinge
color_scale = get(scales, :color, Scale.color_discrete())
Scale.apply_scale(color_scale, [aes], Gadfly.Data(color = hlgroup))
# end
aes.color |
Also test this example (replace the appropriate lines above): scales = Dict{Symbol, Gadfly.ScaleElement}()
color_scale = get(scales, :color, Scale.color_discrete_manual("red","green")) Does it make sense? |
cool, I ran them in my REPL. looks fine. And I just need time to understand what happened. |
This PR is ready for review. I think the Travis CI failures are unrelated to this PR. |
I added an example png. |
docs/src/gallery/geometries.md
Outdated
```@example | ||
using Gadfly, MarketData | ||
set_default_plot_size(21cm, 8cm) | ||
ta = ohlc[1:50] | ||
plot( | ||
x = timestamp(ta), | ||
open = values(ta.Open), | ||
high = values(ta.High), | ||
low = values(ta.Low), | ||
close = values(ta.Close), | ||
Geom.candlestick, | ||
Scale.color_discrete_manual("green", "red") | ||
) | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doc example is identical to the tests. nice things about tests are that they can double as documentation, but are most useful in that capacity only if they present different examples than in the actual docs. how about using one of the large historical data sets from MarketData here instead? i like having real data, instead of synthetic fake data, in docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I will make the example more complete.
I ran into an issue: there are some unwanted gaps on weekend days, if I set the x-axis as a series of Date
. Is there a way to remove that gaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the example in doc.
Codecov Report
@@ Coverage Diff @@
## master #1215 +/- ##
==========================================
+ Coverage 89.32% 89.35% +0.02%
==========================================
Files 39 39
Lines 4395 4404 +9
==========================================
+ Hits 3926 3935 +9
Misses 469 469
Continue to review full report at Codecov.
|
Good to go? |
this looks good to me. @Mattriks ? |
Hopefully I'll get around to looking at this soon (this week). |
any updates? |
Codecov Report
@@ Coverage Diff @@
## master #1215 +/- ##
==========================================
+ Coverage 89.62% 89.64% +0.02%
==========================================
Files 39 39
Lines 4596 4606 +10
==========================================
+ Hits 4119 4129 +10
Misses 477 477
Continue to review full report at Codecov.
|
I resolved the conflict. |
this still LGTM. @Mattriks fine by you? to resolve the conflict, i think the dep on MarketData simply needs to be added to test/Project.toml, and the main Project.toml needs to be left unchanged. |
accidentally merged while resolving conflict, but it looks good so i think it's okay. can always fix later if there's a problem. i couldn't get the docs to build locally though, and the dev version online doesn't seem to have updated. unrelated to this PR though i think. will look into it later. the method signature of
|
thanks @iblislin . sorry this took so long |
🎉 |
Contributor checklist:
squash
'ed orfixup
'ed junk commits with git-rebaseThis PR
Geom.candlestick()
Example
TODO
inverse color, if close price < open priceJust let user set the desired colour viaScale