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

Histogram followup #4195

Merged
merged 12 commits into from
Jul 29, 2019
Merged

Histogram followup #4195

merged 12 commits into from
Jul 29, 2019

Conversation

youri-k
Copy link
Contributor

@youri-k youri-k commented Jul 18, 2019

  • add sampling for float layers and better support for uint24 layers

URL of deployed dev instance (used for testing):

Steps to test:

  • request histogram for float layer => response json should contain min, max entries
  • request histogram for uint24 layer => response json should contain 3 histograms for each channel

Issues:


@youri-k youri-k requested review from fm3, daniel-wer and jstriebel July 18, 2019 14:21
@youri-k youri-k self-assigned this Jul 18, 2019
@youri-k
Copy link
Contributor Author

youri-k commented Jul 18, 2019

@daniel-wer could you do the necessary frontend changes? Please note that the json keys slightly changed and that the backend now always sends an array of histograms.

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

LGTM!

@daniel-wer
Copy link
Member

@philippotto Frontend should be ready for review.
I added histogram support for uint16 and float layers, and fixed the uint24 histograms (they are colored now). (The min/max/minRange/maxRange naming is kind of confusing right now, maybe I should rework this code a bit.)

uint24_histogram

I updated the data_types Dataset to contain a properly scaled uint16 layer now (with values from 0 to 2**16 and not only 2**8) and also another float layer that only contains values between 0 and 1. Please download the updated dataset to properly test this :)

@youri-k Could you also download the updated dataset and have a look at the color_float_small Layer. To me it looks like the min/max range is detected properly, but all the counts are in the first half then (they should be across the whole interval though, I think). Do you agree?

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! Code looks good 👍 The histogram for the RGB dataset might be a bit too overwhelming, but I think it's fine for this iteration.

Sorry, I had the strong urge to scratch the itch coming from #4129. I pushed a small commit to this PR to resolve this. @MichaelBuessemeyer I hope you didn't do this already somewhere else. I wasn't aware that you were assigned to this. Sorry again!

@MichaelBuessemeyer
Copy link
Contributor

@philippotto It did not work on #4129 yet. Thanks for already solving this issue 🙂.

@CLAassistant
Copy link

CLAassistant commented Jul 25, 2019

CLA assistant check
All committers have signed the CLA.

@daniel-wer daniel-wer merged commit f22f6a0 into master Jul 29, 2019
@daniel-wer daniel-wer deleted the histogram-followup branch July 29, 2019 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Histogram followup: Refine float/uint16 layer support
6 participants