Support for displaying a trend line and embedding a right image #24

rahul-deepsource · 2020-09-01T17:41:50Z

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:

Add show-trend argument which takes in comma-separated list of values (maximum: 10).
Add --right-image which would display an image (110 x 14) between left and right text.
Rename --left-color to --bg-color because this was also the background for the right_image, so it didn't really make sense to call it left-color.
Add a new left-color which would add a

Here is an example:

which was generated using the command:

python -m pybadges --left-text "badge trends" --right-text "2.1k" --whole-link "https://deepsource.io/gh/deepsourcelabs/asgard" --bg-color "#252525" --right-color "#63B439" --logo https://raw.githubusercontent.com/deepsourcelabs/brand-assets/master/logo-white.svg --show-trend 1311,2100,3333,598,800,1111,200 --left-color="#4E4E4E" --embed-logo

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! :)

brianquinlan

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/__init__.py

pybadges/__main__.py

brianquinlan · 2020-09-03T20:32:01Z

pybadges/trend.py

+
+
+def fit_data(samples: List[int]) -> Tuple[List[int], List[int]]:
+    y = list(


Does it make sense to do this or to interpolate?
e.g. with numpy.interp

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.

brianquinlan · 2020-09-03T22:12:58Z

pybadges/trend.py

+        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))


So the bottom is always zero?

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.

rahul-deepsource · 2020-09-09T03:47:30Z

Hey, thanks for the review @brianquinlan! Apologies for the delay, I got held up with something else. I'll address the comments today.

rahul-deepsource · 2020-09-09T08:24:54Z

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

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?

brianquinlan · 2020-10-16T17:57:16Z

pybadges/trend.py

+
+
+def normalize(arr: np.ndarray) -> np.ndarray:
+    max_arr = np.max(arr)


Could you write a doc string explaining what this does?

brianquinlan · 2020-10-16T17:57:37Z

pybadges/trend.py

+
+
+def fit_data(samples: List[int]) -> Tuple[List[int], List[int]]:
+    width = WIDTH - X_OFFSET


Could you write a doc string to explain what this does?

brianquinlan · 2020-10-16T17:57:55Z

pybadges/trend.py

+
+
+def trend(samples: List[int], stroke_color: str, stroke_width: int) -> str:
+    canvas = draw.Drawing(WIDTH, HEIGHT, origin=(0, -Y_OFFSET))


Could you write a doc string explaining what this does?

rahul-deepsource · 2020-10-18T20:37:05Z

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.

brianquinlan · 2020-11-07T01:31:09Z

Thanks. I don't have a tonne of bandwidth these days either ;-)

brianquinlan · 2020-12-31T00:11:37Z

OK, I tried patching in this PR and playing with it. Here are some of my thoughts:

We could just decide that this change if out of scope since Github-style badges don't support embedded images. That would be unfortunate.
We can make the API more generic and backwards compatible. We can add new parameters to badge without changing any existing parameters: center_image, center_link, center_color, center_title, embed_center_image.

Additionally, we could add some functionality to:
- Help build the trend line image (but don't change the badge function)
- Make right_text optional i.e. if it is not set then there will be no content on the right side - this would allow you to have the trend line as the right-most element
If we go with this approach, then we could have three smaller (and backwards-compatible) PRs:
1. add center_* support
2. add trend line support
3. make right_text optional

What do you think? I'm happy to help with any of these.

nshan651 · 2022-09-10T22:59:12Z

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.

brianquinlan · 2022-09-13T15:19:47Z

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 ;-)

rahul-deepsource added 4 commits September 1, 2020 20:53

Add ability to embed image to right (or) plot a trend line

443ca2f

Format code with yapf

2bcecc0

change left_color to bg_color in tests

8b5650d

Refactor design; pin dependencies

22f0e14

brianquinlan requested changes Sep 3, 2020

View reviewed changes

rahul-deepsource added 3 commits September 9, 2020 10:46

Improve docstring

83a015a

Stretch plot rather than zero padding

f1b3f76

Import conditionally; Improve docstrings

72a7ccf

brianquinlan reviewed Oct 16, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for displaying a trend line and embedding a right image #24

Support for displaying a trend line and embedding a right image #24

rahul-deepsource commented Sep 1, 2020

brianquinlan left a comment

brianquinlan Sep 3, 2020

rahul-deepsource Sep 9, 2020

brianquinlan Sep 3, 2020

rahul-deepsource Sep 9, 2020

rahul-deepsource commented Sep 9, 2020

rahul-deepsource commented Sep 9, 2020

brianquinlan left a comment

brianquinlan Oct 16, 2020

brianquinlan Oct 16, 2020

brianquinlan Oct 16, 2020

rahul-deepsource commented Oct 18, 2020

brianquinlan commented Nov 7, 2020

brianquinlan commented Dec 31, 2020

nshan651 commented Sep 10, 2022

brianquinlan commented Sep 13, 2022



		def fit_data(samples: List[int]) -> Tuple[List[int], List[int]]:
		y = list(



		def normalize(arr: np.ndarray) -> np.ndarray:
		max_arr = np.max(arr)



		def fit_data(samples: List[int]) -> Tuple[List[int], List[int]]:
		width = WIDTH - X_OFFSET



		def trend(samples: List[int], stroke_color: str, stroke_width: int) -> str:
		canvas = draw.Drawing(WIDTH, HEIGHT, origin=(0, -Y_OFFSET))

Support for displaying a trend line and embedding a right image #24

Are you sure you want to change the base?

Support for displaying a trend line and embedding a right image #24

Conversation

rahul-deepsource commented Sep 1, 2020

brianquinlan left a comment

Choose a reason for hiding this comment

brianquinlan Sep 3, 2020

Choose a reason for hiding this comment

rahul-deepsource Sep 9, 2020

Choose a reason for hiding this comment

brianquinlan Sep 3, 2020

Choose a reason for hiding this comment

rahul-deepsource Sep 9, 2020

Choose a reason for hiding this comment

rahul-deepsource commented Sep 9, 2020

rahul-deepsource commented Sep 9, 2020

brianquinlan left a comment

Choose a reason for hiding this comment

brianquinlan Oct 16, 2020

Choose a reason for hiding this comment

brianquinlan Oct 16, 2020

Choose a reason for hiding this comment

brianquinlan Oct 16, 2020

Choose a reason for hiding this comment

rahul-deepsource commented Oct 18, 2020

brianquinlan commented Nov 7, 2020

brianquinlan commented Dec 31, 2020

nshan651 commented Sep 10, 2022

brianquinlan commented Sep 13, 2022