-
Notifications
You must be signed in to change notification settings - Fork 204
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
Implemented an AutoUnixTimeTicks #313
base: master
Are you sure you want to change the base?
Conversation
eventually. It allows configurability of density of ticks with the `Width`, and aims to be legible on a per-inch basis. Normally, if you set Width to be the same width you render the graph with, things should look good.
} | ||
|
||
if count > 20 { | ||
os.Exit(0) |
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.
that's pretty ugly :)
at the very least, this should be a panic("plot: run away procedure")
.
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.
perhaps just return nil and document?
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.
woohaahh! that was just for debugging :) sorry it went through.. should just take that out.. if you have a sane time range, you won't be hitting that.
ok, first pass done :) looks interesting, thanks! |
|
||
// Inspired by https://github.com/d3/d3-scale/blob/master/src/time.js | ||
var tickIntervals = []tickRule{ | ||
{time.Millisecond, |
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.
Don't break these over two lines.
Folks I'm so sorry to have submitted a half-baked PR like this. I was in a hurry and haven't gotten time to clean it up. I'll do it the moment things calm down over here. |
Simplified rules as timeWindow and durationPerInch were exactly the same in our implementation (it differed in d3's implementation, but we got rid of those diffs).
I am closing this, because we are cleaning up inactive pull requests. See https://groups.google.com/d/topic/gonum-dev/Dr8t8WdvB_4/discussion for more details. |
Folks, could we reopen this ? It's still valid.. don't know why it fell into the cracks.. I'm still using that and I'd be grateful to have it upstream ! |
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 think this probably wants an example.
// adjust the number of ticks according to the specified Width. If | ||
// not specified, Width defaults to 10 centimeters. | ||
type TimeTicks struct { | ||
// Width is the width of the underlying graph, used to calculate |
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.
s/graph/plot/
{10 * year, "2006", "", "2006"}, | ||
} | ||
|
||
const month = 31 * 24 * time.Hour |
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.
These can be in a const block. Also, I'm sort of troubled by the choice of length of month; the marginally closer length is 30 days.
// shown each time the timestamp goes over a certain boundary | ||
// (verified through `watchFormat`). This way you can show `Sep 2, | ||
// 12pm` when you pass midnight after `11pm` on `Sep 1`. | ||
type tickRule struct { |
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 type does not need to exist. You can define tickRules
on the anonymous struct.
func (t TimeTicks) Ticks(min, max float64) []Tick { | ||
width := t.Width | ||
if width == 0 { | ||
width = 10 * vg.Centimeter |
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.
What is the basis for this?
// (verified through `watchFormat`). This way you can show `Sep 2, | ||
// 12pm` when you pass midnight after `11pm` on `Sep 1`. | ||
type tickRule struct { | ||
durationPerInch time.Duration // use this rule for a maximum Duration per inch, it is also used as an interval per ticks. |
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.
SI please.
durationPerInch := maxT.Sub(minT) / time.Duration(width/vg.Inch) | ||
|
||
lastElement := len(tickRules) - 1 | ||
rule := tickRules[lastElement] |
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.
Just use rule := tickRules[len(tickRules)-1]
and similarly in the range.
var ticks []Tick | ||
for { | ||
if delta > 0 { | ||
// Count in Months now |
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.
Full stop.
if lastWatch == newWatch { | ||
label = start.Format(rule.shortFormat) | ||
} else { | ||
//TODO: overwrite the first tick with the long form if we |
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.
s|//TODO: o|// TODO(name): O|
label = start.Format(rule.shortFormat) | ||
} else { | ||
//TODO: overwrite the first tick with the long form if we | ||
// haven't shown a lonform at all.. instead of always |
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.
s/lonform/longform/
s/../,/
want: []string{"2017", "2022", "2027", "2032", "2037", "2042", "2047"}, | ||
}, | ||
} { | ||
//fmt.Println("For dates", test.min, test.max) |
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.
Delete this line.
Could probably be the default eventually.
It allows configurability of density of ticks with the
Width
, and aimsto be legible on a per-inch basis. Normally, if you set Width to be
the same width you render the graph with, things should look good.
Check tests for a feel of what it would look like once rendered (for 4 inch-wide graph).