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

Fix histogram scale #4926

Merged
merged 13 commits into from
Nov 23, 2020
Merged

Fix histogram scale #4926

merged 13 commits into from
Nov 23, 2020

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Nov 9, 2020

This PR fixes the histogram scale. Previously we downscaled the data before applying the logarithm. This leads to a linear scale.
With this fix, the logarithm is applied before scaling the data to the histogram height.

During testing, I found 2 errors in the console produced by the new downscaling modal added by the multi-resolution PR. I just fixed those on the fly.

URL of deployed dev instance (used for testing):

Steps to test:

  • Open a dataset and look at the histogram. Come this with the master. The small values should now be much higher in the histogram. The details in the data should also be increased for small values.
  • As I also fiddled with the modal mentioned above please check whether it looks alright and does not throw errors anymore.
  • For that: Open a only tracing that did not get downscaled on your local machine and then open the modal using the orange icon above the segmentation layer settings in the tracing view.

Issues:


@MichaelBuessemeyer
Copy link
Contributor Author

After a discussion, we decided to use a linear scale 📈

@MichaelBuessemeyer
Copy link
Contributor Author

@daniel-wer I got an exciting update. The scale is now linear and there are now even values on the y-axis 🤯
image

Could you please check out my changes?

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Nov 16, 2020

A quick internal discussion stated that we do not need the y-axis as the values are sampled and as the scale now is linear (which is what a user should expect)

Nevertheless, could you please check out this PR?

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Thanks @MichaelBuessemeyer for fixing this and reacting dynamically to the feedback others and I gave you 🙏

One weird thing I noticed on this branch, but not on master. At first the histogram line is thin, but when moving a histogram handle for the first time, the line becomes thicker all of a sudden and there's also a line at the bottom. See this gif:
histo_width

@MichaelBuessemeyer
Copy link
Contributor Author

One weird thing I noticed on this branch, but not on master. At first the histogram line is thin, but when moving a histogram handle for the first time, the line becomes thicker all of a sudden and there's also a line at the bottom.

Oh, thanks for finding this bug. This was caused, because the canvas always used the same path. Refreshing was done in the drawYAxis method. I just added using a new path directly to the beginning of the drawHistogram method to fix this 👍

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice find with the fix for the double line drawing bug 💯

@MichaelBuessemeyer MichaelBuessemeyer merged commit 3e07a3c into master Nov 23, 2020
@fm3 fm3 deleted the fix-histogram-scale branch January 18, 2021 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix histogram scale
2 participants