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

adds y-axis scale to the histogram view #4349

Merged
merged 7 commits into from
Dec 10, 2019

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Dec 2, 2019

URL of deployed dev instance (used for testing):

Steps to test:

  • open a tracing
  • open the datasets tab in the settings
  • the histogram should now have a scale at the y-axis indicating the logarithmic scale

Issues:


@philippotto
Copy link
Member

Thanks for adding this scale! However, I'd vote for a more visual approach (without specific numbers). The numbers don't add much value (since they depend on a lot of factors, such as sample size, and cannot really be interpreted).

Instead, I'd suggest to only draw some horizontal lines which indicate the logarithm. Similar to the following pattern, multiple horizontal lines could be directly drawn into the canvas in a light blue (or gray) tone.

image

You can draw the lines by dividing the range of the histogram values into n equally-sized intervals (e.g., if the histogram covers values 0 to 1000; you could divide it into the ranges 0-200, 200-400, 400-600, 600-800, 800-1000). For the upper values of each interval, you calculate it's height in the logarithmic scale and draw the line there. This should reproduce the logarithmic visual from the picture above.

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Dec 3, 2019

Following your suggestion I could produce the following:

image

As there are almost only lines a the top of the axis, this does not look nice to me.

I reworked this a bit to make this look more appealing. Now due to numeric instability the histogram looks quite different:
image

my conclusion: either the histogram or the axis scale do not look so good.
Let's talk about this a bit.

@philippotto
Copy link
Member

I reworked this a bit to make this look more appealing. Now due to numeric instability the histogram looks quite different:

Why do you need to change the histogram itself? I would have thought, that you could simply divide the maximum value in 4 ranges and then you only have 3 lines on the axis. But let's talk about this in person!

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Dec 3, 2019

I just checked why there is a > comparison instead of >=. The reason is pretty simple: The logarithm of 0 equals - infinity. Therefore this check is correct. Maybe a comment would be helpful to clarify this :).
I am adding the comment in PR #4349

  • [ ] Done -> Outdated due to code changes.

* smoothing is done by reducing the intensity value into the range
  from 1 to 10 and then taking the logarithm of these values
@MichaelBuessemeyer
Copy link
Contributor Author

We agreed on the second version (2nd picture)

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto Everything should be ready for your second review. Lets wait for the CI to deploy a dev instance 👍

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto I added the dev instance :)

@philippotto
Copy link
Member

Awesome! Will review in a bit :)

@fm3 In #4349 (comment) it seems as if the normalization and logarithmic transformation helps to bring out a two-gaussian-characteristic. Maybe this insight helps when performing a histogram normalization? Not sure, whether this is helpful, but it just crossed my mind.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Just see my few nitpicks before merging :)

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -122,7 +140,7 @@ class Histogram extends React.PureComponent<HistogramProps> {
ref={ref => {
this.canvasRef = ref;
}}
width={300}
width={canvasWidth}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 ^^

// We use canvasHeight - 1 because else half of the top line would be cut off.
const lineHeight = Math.round(Math.log10(intervalSize * interval) * (canvasHeight - 1));
ctx.moveTo(0, lineHeight);
ctx.lineTo(8, lineHeight);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extract the 8 as a lineWidth variable?

@MichaelBuessemeyer MichaelBuessemeyer merged commit 649fbdb into master Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add scale to y-axis of histogram
2 participants