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

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

Merged
merged 19 commits into from Mar 19, 2019

Conversation

@bobheadxi
Copy link
Contributor

commented Mar 17, 2019

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: #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
@bobheadxi bobheadxi force-pushed the bobheadxi:master branch from 4b73a91 to 669f33c Mar 17, 2019
Signed-off-by: Robert Lin <robertlin1@gmail.com>
@bobheadxi bobheadxi force-pushed the bobheadxi:master branch from 669f33c to 351c12e Mar 17, 2019
@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

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 bobheadxi force-pushed the bobheadxi:master branch from d626d10 to d232879 Mar 17, 2019
@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

@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

image

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

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.
Copy link
Contributor

left a comment

This was a huge amount of work πŸ‘ ! Nothing breaking in the review comments.

contrib/_plugin_example/churn_analysis.go Outdated Show resolved Hide resolved
ticks.tickSize = time.Duration(val) * time.Hour
} else {
// default to 1 day
ticks.tickSize = 24 * time.Hour

This comment has been minimized.

Copy link
@vmarkovtsev

vmarkovtsev Mar 18, 2019

Contributor

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.

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Mar 18, 2019

Author Contributor

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

internal/plumbing/ticks.go Outdated Show resolved Hide resolved
internal/plumbing/ticks.go Show resolved Hide resolved
leaves/burndown.go Show resolved Hide resolved
internal/plumbing/ticks.go Show resolved Hide resolved
bobheadxi added 7 commits Mar 18, 2019
Signed-off-by: Robert Lin <robertlin1@gmail.com>
Signed-off-by: Robert Lin <robertlin1@gmail.com>
Signed-off-by: Robert Lin <robertlin1@gmail.com>
Signed-off-by: Robert Lin <robertlin1@gmail.com>
Signed-off-by: Robert Lin <robertlin1@gmail.com>
Signed-off-by: Robert Lin <robertlin1@gmail.com>
@bobheadxi bobheadxi force-pushed the bobheadxi:master branch from ef1a11f to 77cf86a Mar 18, 2019
@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

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
	}
bobheadxi added 2 commits Mar 18, 2019
Signed-off-by: Robert Lin <robertlin1@gmail.com>
Signed-off-by: Robert Lin <robertlin1@gmail.com>
@vmarkovtsev

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

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>
bobheadxi added 4 commits Mar 18, 2019
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>
Signed-off-by: Robert Lin <robertlin1@gmail.com>
}
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) {

This comment has been minimized.

Copy link
@vmarkovtsev

vmarkovtsev Mar 19, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Pending tasks:

  1. Regenerate doc/dag.png for the README header.
  2. Update labours.py to read the tick size and adjust the X axes.
  3. 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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

  1. I've updated the diagram (bc05602)
  2. 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? πŸ™
  3. πŸš€

thanks for the reviews and help @vmarkovtsev !

@vmarkovtsev vmarkovtsev merged commit 0839cec into src-d:master Mar 19, 2019
4 checks passed
4 checks passed
DCO DCO
Details
Travis CI - Pull Request Build Passed
Details
codecov/project Absolute coverage decreased by -0.12% but relative coverage increased by +4.75% compared to c8fd37d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.