Add more granular intervals (replace 'days' with generic 'ticks') by bobheadxi · Pull Request #245 · src-d/hercules

bobheadxi · 2019-03-17T07:34:41Z

closes #238 , more context is in the issue

Seeking feedback! 🙏 probably still needs a bit of work

this PR changes all mentions of "days" to more flexible "ticks", currently configured by ConfigTicksSinceStartTicksSize (or --tick-size) that accepts an integer value as the number of hours, though tick size is actually tracked as time.Duration so even more granular options could be exposed (though likely doesn't need to)

internal/plumbing/day.go was renamed to ticks.go which unfortunately makes the diff a bit hard to review, sorry!

default tick size

sets to 24 hours as before, so hopefully it's not significantly breaking

❯ hercules --granularity=5 --sampling=5 --burndown https://github.com/launchpals/open-now
hercules:
  version: 9
  hash: 6c50214b59f32b8714469380e1ffcd0cb044517d
  repository: https://github.com/launchpals/open-now
  begin_unix_time: 1548537944
  end_unix_time: 1548645154
  commits: 83
  run_time: 126
Burndown:
  granularity: 5
  sampling: 5
  "project": |-
    472777

custom tick size

for example, with 1 hour:

❯ hercules --granularity=5 --sampling=5 --tick-size=1 --burndown --first-parent https://github.com/launchpals/open-now 
hercules:
  version: 9
  hash: 6c50214b59f32b8714469380e1ffcd0cb044517d
  repository: https://github.com/launchpals/open-now
  begin_unix_time: 1548537944
  end_unix_time: 1548645154
  commits: 10
  run_time: 47
Burndown:
  granularity: 5
  sampling: 5
  "project": |-
    1531        0      0      0      0      0
      1531      0      0      0      0      0
      1258      0    960      0      0      0
      1258      0    960      0      0      0
      1255      0    945      0 468480      0
      1255      0    942      0 468450   2076

bugs

any pointers on these would be appreciated!

a few failing tests I don't really understand: Add more granular intervals (replace 'days' with generic 'ticks') #245 (comment)
can't seem to get labours to work, but I havent investigated it yet:

edit works with hercules --burndown --first-parent --tick-size=1 --pb https://github.com/bobheadxi/calories | python3 labours.py -f pb -m burndown-project, I think it's an issue with small repos. that said, the timescales dont work properly with anything other than --tick-size=24 for now.

~/go/src/gopkg.in/src-d/hercules.v9 master* ⇡
❯ hercules --granularity=3 --sampling=3 --tick-size=1 --burndown --pb https://github.com/launchpals/open-now | python3 labours.py -f pb -m burndown-projectdoneing...
project lifetime index: 0.20172258097493653
resampling to year, please wait...
Traceback (most recent call last):
  File "labours.py", line 1942, in <module>
    sys.exit(main())
  File "labours.py", line 1913, in main
    modes[args.mode]()
  File "labours.py", line 1759, in project_burndown
    resample=args.resample))
  File "labours.py", line 627, in load_burndown
    daily[istart:ifinish, (sdt - start).days:].sum(axis=0)
UnboundLocalError: local variable 'sdt' referenced before assignment

~/go/src/gopkg.in/src-d/hercules.v9 master ⇡
❯ hercules --granularity=3 --sampling=3  --burndown --pb https://github.com/launchpals/open-now | python3 labours.py -f pb -m burndown-project 
doneing...
Traceback (most recent call last):
  File "labours.py", line 1942, in <module>
    sys.exit(main())
  File "labours.py", line 1913, in main
    modes[args.mode]()
  File "labours.py", line 1759, in project_burndown
    resample=args.resample))
  File "labours.py", line 589, in load_burndown
    print(name, "lifetime index:", calculate_average_lifetime(matrix))
  File "labours.py", line 439, in calculate_average_lifetime
    lifetimes[i - start] = band[i - 1]
IndexError: index -1 is out of bounds for axis 0 with size 0

Signed-off-by: Robert Lin <robertlin1@gmail.com>

bobheadxi · 2019-03-17T07:42:01Z

awkward, really messed up following the DCO bot's suggestion... squashed everything into a single commit and force-pushed 😅

Signed-off-by: Robert Lin <robertlin1@gmail.com>

bobheadxi · 2019-03-17T08:46:14Z

@vmarkovtsev , which version of protoc and protoc-gen-go do you use? When I build the proto files I seem to get pretty significant diffs.

libprotoc 3.6.1
protoc-gen-go 1.3.1

Signed-off-by: Robert Lin <robertlin1@gmail.com>

bobheadxi · 2019-03-17T08:55:33Z

a few comments on the currently failing tests:

the pipeline serialization seems to be outputting something different that doesn't seem directly related to the Days/Ticks change
for one of the TicksSinceStart (formerly DaysSinceStart) tests:

--- FAIL: TestTicksSinceStartConsumeZero (0.00s)
	ticks_test.go:210: 
			Error Trace:	ticks_test.go:210
			Error:      	Not equal: 
			            	expected: 1970
			            	actual  : 1969
			Test:       	TestTicksSinceStartConsumeZero

on my machine, I get 1969:

Running tool: /usr/local/bin/go test -timeout 60s gopkg.in/src-d/hercules.v9/internal/plumbing -run ^(TestTicksSinceStartConsumeZero)$ -v

=== RUN   TestTicksSinceStartConsumeZero
--- PASS: TestTicksSinceStartConsumeZero (0.00s)
PASS
ok  	gopkg.in/src-d/hercules.v9/internal/plumbing	0.109s
Success: Tests passed.

vmarkovtsev

This was a huge amount of work 👍 ! Nothing breaking in the review comments.

vmarkovtsev · 2019-03-18T15:13:30Z

+		ticks.tickSize = time.Duration(val) * time.Hour
+	} else {
+		// default to 1 day
+		ticks.tickSize = 24 * time.Hour


We should check if tickSize is 0 inside Initialize() and set it to the default if it is. If there wasn't ConfigTicksSinceStartTickSize, we should change the current value to the default only if it equals to 0.

I've adjusted this in 1143e52 (and corrected again in d9e5517), is that how you want it? a little confused between when to set things in configure, and when to do it on initialize

Signed-off-by: Robert Lin <robertlin1@gmail.com>

bobheadxi · 2019-03-18T18:10:38Z

@vmarkovtsev I realized in all this I missed actually incorporating the ticksize into the burndown analysis 😅 so I just had a shot at it in 77cf86a - is there a test I didn't update that I should update / write to verify that this behaves properly for various tickSize? Thanks!

Also wondering if you saw my other comment re: my protobuf generation being significantly different from yours, in this comment: #245 (comment), and the failing tests in #245 (comment)

vmarkovtsev · 2019-03-18T19:01:02Z

Regarding pb.pb.go, I use protoc-gen-gogo. Refer to Makefile.

Regarding 1969 vs 1970, the difference is due to the timezones. I cannot advise what exactly should be changed, but it is definitely because the European timezone is mishandled.

Regarding testing tickSize, apart from serialization and deserialization, there should be an error test for merging BurndownResult with different tick sizes.

Signed-off-by: Robert Lin <robertlin1@gmail.com>

bobheadxi · 2019-03-18T21:23:23Z

Regarding pb.pb.go, I use protoc-gen-gogo. Refer to Makefile.

Yep, I can generate it correctly using the Makefile - but I think there's either a bit of a version mismatch, or I am doing something wrong. I have protoc-gen-gogo v1.2.1 installed, and running make internal/pb/pb.pb.go I still end up with strangely large diffs (over 1000 lines), which seems suspicious. If that's fine I guess it can be ignored 👌

Just a thought, but with generated code I usually add a CI job to generate code using the expected version of the tools, and then use git to exit out if an unexpected diff is detected. Example: https://github.com/ubclaunchpad/pinpoint/blob/master/.travis.yml#L21

Regarding testing tickSize, apart from serialization and deserialization, there should be an error test for merging BurndownResult with different tick sizes.

Is the expected behaviour for merging different tick sizes to error? It currently doesn't, because I added this:

	if bar1.TickSize < bar2.TickSize {
		merged.TickSize = bar1.TickSize
	} else {
		merged.TickSize = bar2.TickSize
	}

Signed-off-by: Robert Lin <robertlin1@gmail.com>

vmarkovtsev · 2019-03-18T21:49:06Z

Different tick sizes do not contradict with result merging, but they add complexity to the code which is already complex as hell. We cannot just set the tick size because we need to resample the matrices, and that's not short. So if the tick sizes are different, we should return an error for now.

vmarkovtsev · 2019-03-18T21:52:34Z

I installed protoc-gen-gogo about 2 years ago, and I doubt that I have ever updated it since then 😄. So let's change Makefile to work with a specific version and regenerate files with it.

bobheadxi · 2019-03-18T23:15:52Z

So let's change Makefile to work with a specific version and regenerate files with it.

Probably best to leave this for another PR, if that's okay - we'll want to bump protobuf dependencies, and a bit of scripting will be needed as well (since go get plays poorly with versions 😞, so we'll need to cd into directories and fiddle around with things). I'll open a ticket! (#247)

Signed-off-by: Robert Lin <robertlin1@gmail.com>

…ences Signed-off-by: Robert Lin <robertlin1@gmail.com>

Signed-off-by: Robert Lin <robertlin1@gmail.com>

vmarkovtsev · 2019-03-19T13:07:01Z

 	assert.Equal(t, res[DependencyTick].(int), 0)
 	assert.Equal(t, tss.previousTick, 0)
-	assert.Equal(t, tss.tick0.Year(), 1969)
+	if (tss.tick0.Year() != 1969) && (tss.tick0.Year() != 1970) {


The root of the problems seems to be that commit.Committer.When is initialized to time.Unix(0, 0) and that time must be GMT. It is subsequently converted to the local time and we get 1969. Should be fine here.

vmarkovtsev · 2019-03-19T13:08:58Z

Pending tasks:

Regenerate doc/dag.png for the README header.
Update labours.py to read the tick size and adjust the X axes.
hercules.v9 -> hercules.v10 - should be in a separate PR, I will handle it myself.

I can take care of (2) if you wish.

Signed-off-by: Robert Lin <robertlin1@gmail.com>

bobheadxi · 2019-03-19T19:05:56Z

I've updated the diagram (bc05602)
Unfortunately I'm going to be busy this week, so won't be able to look into labours for a bit - if you don't mind, can you take care of (2), or leave is as a separate issue and I can take a look at it next week? 🙏
🚀

thanks for the reviews and help @vmarkovtsev !

Add more granular intervals (replace 'days' with generic 'ticks')

Rename 'days' to 'ticks', add tick-size configuration

351c12e

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Update plugin example, sentiment, and devs pipelines for 'ticks'

d232879

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Update serialization tests for new 'ticks' wording

1299bc0

Signed-off-by: Robert Lin <robertlin1@gmail.com>

bobheadxi mentioned this pull request Mar 17, 2019

analysis: scaling burndowns for short-term (eg hackathon) vs long-term (eg linux) projects bobheadxi/timelines#27

Closed

3 tasks

vmarkovtsev requested changes Mar 18, 2019

View reviewed changes

bobheadxi added 7 commits March 18, 2019 10:09

Merge branch 'master' of https://github.com/src-d/hercules

0cc6166

Fix awkward typo

a9b3dfb

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Fix TickSize initialization

1143e52

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Make TickSize public

c0567f8

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Break on commit seek when found

b4af94f

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Add TickSize to facts, burndown analyser and burndown results

5b3a96b

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Incorporate tickSize into burndown calculations

77cf86a

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Regenerate pb.go with protoc-gen-gogo@v1.2.1

e34b0db

Signed-off-by: Robert Lin <robertlin1@gmail.com>

bobheadxi added 2 commits March 18, 2019 14:28

Update pipeline serialization test

d4ef2dd

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Fix tickSize initialization

d9e5517

Signed-off-by: Robert Lin <robertlin1@gmail.com>

bobheadxi mentioned this pull request Mar 18, 2019

update protobuf dependencies and enforce tooling versions via make #247

Open

bobheadxi added 5 commits March 18, 2019 16:22

Return error if burndown tick sizes are different

682d687

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Add tick size to burndown text serialization

90fec58

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Make 0-value tick test more lenient towards potential timezone differ…

fd50cdc

…ences Signed-off-by: Robert Lin <robertlin1@gmail.com>

Fix burndown merge with zero-value analyser

9da202b

Signed-off-by: Robert Lin <robertlin1@gmail.com>

Fix days reference

f16eaf8

Signed-off-by: Robert Lin <robertlin1@gmail.com>

vmarkovtsev approved these changes Mar 19, 2019

View reviewed changes

vmarkovtsev reviewed Mar 19, 2019

View reviewed changes

Update pipeline DAG

bc05602

Signed-off-by: Robert Lin <robertlin1@gmail.com>

vmarkovtsev merged commit 0839cec into src-d:master Mar 19, 2019

This was referenced Mar 23, 2019

Expose plumbing.ConfigTicksSinceStartTickSize #259

Merged

burndown with granularity != sampling panics #260

Open

dmytrogajewski pushed a commit to dmytrogajewski/hercules that referenced this pull request Jul 28, 2025

Merge pull request src-d#245 from bobheadxi/master

104dce9

Add more granular intervals (replace 'days' with generic 'ticks')

CWBudde pushed a commit to CWBudde/hercules that referenced this pull request Apr 10, 2026

Merge pull request src-d#245 from bobheadxi/master

0b28fd1

Add more granular intervals (replace 'days' with generic 'ticks')

Conversation

bobheadxi commented Mar 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

default tick size

custom tick size

bugs

Uh oh!

bobheadxi commented Mar 17, 2019

Uh oh!

bobheadxi commented Mar 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobheadxi commented Mar 17, 2019

Uh oh!

vmarkovtsev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vmarkovtsev Mar 18, 2019

Choose a reason for hiding this comment

Uh oh!

bobheadxi Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bobheadxi commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vmarkovtsev commented Mar 18, 2019

Uh oh!

bobheadxi commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vmarkovtsev commented Mar 18, 2019

Uh oh!

vmarkovtsev commented Mar 18, 2019

Uh oh!

bobheadxi commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vmarkovtsev Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

vmarkovtsev commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobheadxi commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bobheadxi commented Mar 17, 2019 •

edited

Loading

bobheadxi commented Mar 17, 2019 •

edited

Loading

bobheadxi Mar 18, 2019 •

edited

Loading

bobheadxi commented Mar 18, 2019 •

edited

Loading

bobheadxi commented Mar 18, 2019 •

edited

Loading

bobheadxi commented Mar 18, 2019 •

edited

Loading

vmarkovtsev commented Mar 19, 2019 •

edited

Loading

bobheadxi commented Mar 19, 2019 •

edited

Loading