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

Add histogram to dataset settings #4105

Merged
merged 26 commits into from
Jun 11, 2019

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented May 22, 2019

This PR adds the long-awaited histogram feature for to the dataset settings. It uses the new histogram backend route to get the sampled histogram data of the current dataset.

URL of deployed dev instance (used for testing):

Steps to test:

  • Open some tracing
  • There open the dataset settings
  • Each layer should have its own histogram. With this, you can adjust the max and min value.

Issues:

ToDo:


@philippotto
Copy link
Member

@MichaelBuessemeyer can you add the remaining to dos in form of checkboxes to this PR? Or is everything done and ready for review? :)

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented May 28, 2019

@philippotto As we can't convert brightness and contrast into a good approximation of the intensityRange (min and max value) and we no longer support brightness and contrast: Do you thing just replacing brightness and contrast in the dataset layer settings is fine? I think @daniel-wer also suggested this. And using default values should be fine as our customer mentioned this in the discuss 🙂

Maybe last TODOs:

  • Automatically replace the brightness and contrast parameter in the default dataset layer settings of a dataset with an intensityRange of [0, 255]
  • [ ] Add intensityRange to the required values for valid dataset layer settings

@philippotto
Copy link
Member

As we can't convert brightness and contrast into a good approximation of the intensityRange (min and max value) and we no longer support brightness and contrast: Do you thing just replacing brightness and contrast in the dataset layer settings is fine? I think @daniel-wer also suggested this. And using default values should be fine as our customer mentioned this in the discuss slightly_smiling_face

Ok, sounds good!

  • Automatically replace the brightness and contrast parameter in the default dataset layer settings of a dataset with an intensityRange of [0, 255]
  • Add intensityRange to the required values for valid dataset layer settings

Regarding the second todo: the value should be valid and accepted, but it shouldn't be required. Otherwise, all existing dataset layer settings will become immediately invalid when we deploy this. Instead, the old configurations should still be valid (so, the old brightness/contrast values are still valid and ignored and the new intensityRange is "optional").

type Vector3 = Array<number>;
type Vector3 = [number, number, number];

type Vector2 = [number, number];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think defining the Vectors as tuples is more accurate as it defines the length of the array

@@ -5,3 +5,7 @@
.centered-children {
text-align: center;
}

.tracing-settings-menu .ant-tabs-content {
padding-right: 8px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some padding to the settings on the left to not get elements covered by the scrolling bar.

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto This PR is ready for your review 🎉

@MichaelBuessemeyer MichaelBuessemeyer changed the title [WIP] Add histogram to dataset settings Add histogram to dataset settings Jun 1, 2019
@philippotto philippotto self-requested a review June 3, 2019 08:52
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.

Nice stuff! Code looks good to me. Only two things:

  • For float layers, we said that we won't support the histogram in the first iteration, right? Then, I'd hide the histogram for those layers. Right now, the user can use the slider, but it won't change anything. Also, the histogram doesn't look right due to the range of 0-255.
  • When disabling a layer, the min-max-slider should be disabled (and I'd argue that the histogram could also be hidden)

@MichaelBuessemeyer
Copy link
Contributor Author

  • For float layers, we said that we won't support the histogram in the first iteration, right? Then, I'd hide the histogram for those layers. Right now, the user can use the slider, but it won't change anything. Also, the histogram doesn't look right due to the range of 0-255.
  • When disabling a layer, the min-max-slider should be disabled (and I'd argue that the histogram could also be hidden)

@philippotto Thanks for paying attention to those points.
Regarding the second point, I also agree that if you have a layer disabled, the histogram of that layer should be hidden. This makes the settings more compact.

Please note: As I do not have a dataset with float type I cannot try out the first point.

And you can give this PR another of your awesome reviews :)

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, looks good to me 👍

@philippotto
Copy link
Member

@MichaelBuessemeyer This can be merged, right?

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto Indeed, It can.
I can't work on possible fixes today, but this should be alright 🙂
And thanks for updating this branch

@MichaelBuessemeyer MichaelBuessemeyer merged commit 2a81892 into master Jun 11, 2019
@normanrz normanrz deleted the add-histogram-to-dataset-settings branch August 12, 2019 09:46
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.

(Auto) min-max to determine brightness / contrast
2 participants