Support for displaying a trend line and embedding a right image#24
Support for displaying a trend line and embedding a right image#24rahul-deepsource wants to merge 7 commits intogoogle:masterfrom
Conversation
brianquinlan
left a comment
brianquinlan
left a comment
There was a problem hiding this comment.
This is super cool! A few things:
- could you add tests for trend.py?
- could you add some golden images and the end to end tests for them?
- could you increase the version number by a major version?
- could you update the README (and add some of your examples!)
Once again: amazing work!
pybadges/trend.py
Outdated
|
|
||
|
|
||
| def fit_data(samples: List[int]) -> Tuple[List[int], List[int]]: | ||
| y = list( |
There was a problem hiding this comment.
Does it make sense to do this or to interpolate?
e.g. with numpy.interp
There was a problem hiding this comment.
The reason behind repeating these numbers it that the more samples we have, we can increase the polyfit order more liberally which smoothens the curve considerably.
I don't think that it would make much sense to first interpolate the curve and then fit it again.
Also, I found out about np.repeat. Removing this hacky repeat method.
| itertools.chain.from_iterable( | ||
| itertools.repeat(sample, 10) for sample in samples)) | ||
| xp = np.arange(len(y)) | ||
| yp = normalize(np.poly1d(np.polyfit(xp, y, 15))(xp)) |
There was a problem hiding this comment.
So the bottom is always zero?
There was a problem hiding this comment.
Ahm, I don't quite get the question.
If all the values in yp are equal, then we'd have an array of all 1's after normalization. Similarly for all zeroes, we'd have a flat line.
Polyfit might return negative numbers sometime (-1 at most). But, since we only scale up positive numbers (yp[yp > 0] *= ...), and use an offset of -1 when defining the origin, it shouldn't be a problem.
|
Hey, thanks for the review @brianquinlan! Apologies for the delay, I got held up with something else. I'll address the comments today. |
|
I have another concern: the inline images in the SVG don't show up while viewing the raw image (served by https://raw.githubusercontent.com). For e.g., the image I linked in the example above looks like this: Firefox: Brave (Blink more generally), we get two ugly broken image icons:
I am not sure why this happens -- my guess is the Same origin policy? Is there a way to resolve this? |
brianquinlan
left a comment
brianquinlan
left a comment
There was a problem hiding this comment.
I'm really sorry for the slow review process - life is kind of crazy right now ;-)
Could you add tests for trend.py, an example for test-badge.json, increment the major version number and README page?
|
|
||
|
|
||
| def normalize(arr: np.ndarray) -> np.ndarray: | ||
| max_arr = np.max(arr) |
There was a problem hiding this comment.
Could you write a doc string explaining what this does?
|
|
||
|
|
||
| def fit_data(samples: List[int]) -> Tuple[List[int], List[int]]: | ||
| width = WIDTH - X_OFFSET |
There was a problem hiding this comment.
Could you write a doc string to explain what this does?
|
|
||
|
|
||
| def trend(samples: List[int], stroke_color: str, stroke_width: int) -> str: | ||
| canvas = draw.Drawing(WIDTH, HEIGHT, origin=(0, -Y_OFFSET)) |
There was a problem hiding this comment.
Could you write a doc string explaining what this does?
|
Hey @brianquinlan! No worries! I can totally understand! I'm leaving this comment to show a sign of life, and that I'm still interested in getting this through. However, I won't be able to work on it atleast until the end of this week. I'll update it soon. |
|
Thanks. I don't have a tonne of bandwidth these days either ;-) |
|
OK, I tried patching in this PR and playing with it. Here are some of my thoughts:
What do you think? I'm happy to help with any of these. |
|
Hello, I'm interested in helping out with making the pybadges API more generic with support for a center parameter. I'm working on project for creating longitudinal status badges of CI build metrics, and I think a more generic version of pybadges would be really useful! Please let me know if this is still in line with the project vision. |
Hey @nshan651 , I'd be happy to make pybadges more generic! I think that this PR just stalled because the author ran out of bandwidth and I'll pretty picky about complex PRs ;-) |
3 participants
Thanks for the wonderful library!
I wanted to add the functionality to add a trend line which can graphically show how a metric has been varying over time, and thought that this would be a nice addition to this library as well!
Here are a bunch of changes:
show-trendargument which takes in comma-separated list of values (maximum: 10).--right-imagewhich would display an image (110 x 14) between left and right text.--left-colorto--bg-colorbecause this was also the background for theright_image, so it didn't really make sense to call itleft-color.left-colorwhich would add aHere is an example:
which was generated using the command:
There are still a few tests failing (and docs missing). If you give me a green flag that you intend to merge these changes, I'd be happy to provide them as well! :)