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.

Sign up for GitHub

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

Jump to bottom

Fix initial min/max values of a histogram of a color layer #5853

Merged
merged 7 commits into from
Nov 22, 2021
Merged

Fix initial min/max values of a histogram of a color layer #5853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
58ba960
enforce intensityRange of a color layer to be within min and max
MichaelBuessemeyer Nov 15, 2021
eead15b
Merge branch 'master' into fix-histogram-slider
MichaelBuessemeyer Nov 15, 2021
44b7bbe
simplify code
MichaelBuessemeyer Nov 15, 2021
97a4916
add changelog entry
MichaelBuessemeyer Nov 15, 2021
a86bf9f
Merge branch 'master' of github.com:scalableminds/webknossos into fix…
MichaelBuessemeyer Nov 22, 2021
0525da5
add comment explaining the changes
MichaelBuessemeyer Nov 22, 2021
d24fbdf
Apply suggestions from code review
philippotto Nov 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- The docker setup has been restructured, which requires changes to existing docker-compose setups. See the migration guide for details. [#5843](https://github.com/scalableminds/webknossos/pull/5843)

### Fixed
- Fixed a bug that the displayed value range of a histogram of a color layer wasn't applied until the slider was dragged a bit. [#5853](https://github.com/scalableminds/webknossos/pull/5853)
- Fixed a bug where admins could not share annotations with teams they were not explicitly a member of. [#5845](https://github.com/scalableminds/webknossos/pull/5845)

### Removed
Expand Down
21 changes: 16 additions & 5 deletions frontend/javascripts/oxalis/model/sagas/load_histogram_data_saga.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,32 @@ export default function* loadHistogramData(): Saga<void> {
const minimumInHistogramData = Math.min(...allMinValues);
const maximumInHistogramData = Math.max(...allMaxValues);
let newIntensityRange = [];
if (layerConfigurations[layerName]) {
const currentLayerConfig = layerConfigurations[layerName];
if (currentLayerConfig) {
// The intensityRange is the range where the color value will be clamped to
// while min/max are the value range of the histogram slider.
// The following keeps the intensityRange within the min and max bounds if they exist.
// If they do not exist the tracing / dataset has never been opened and thus the histogram data is used as a default range delimiter.
newIntensityRange = [
Math.max(layerConfigurations[layerName].intensityRange[0], minimumInHistogramData),
Math.min(layerConfigurations[layerName].intensityRange[1], maximumInHistogramData),
Math.max(
currentLayerConfig.intensityRange[0],
currentLayerConfig.min != null ? currentLayerConfig.min : minimumInHistogramData,
),
Comment on lines +56 to +59
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment which explains the intuition here? I think, I kind of got now, but I had think this through for a few minutes and I'm still not 100% sure. My take would be:
intensityRange is the range for which the histogram is plotted, right? By default, it's defined by the data type (e.g., uint16), but it can be configured per dataset layer.
That range serves as a boundary (i.e., the range cannot be increased by something else), but can still be narrowed down. If nothing else is provided, that down-narrowing is done by the sampled histogram. If the user specified a default min/max value, that is used instead.

Also interesting: intensityRange is more like a "theoretical" range which is not exceeded by the data (e.g., the microscope did only emit values between x and y), whereas min/max is more like a recommended cut-off to get a good rendering?

Did I get this right? 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

intensityRange is the range for which the histogram is plotted, right?

It is the exact opposite. The min/max is the range of the histogram (slider) while the intensityRange is the range to which the color values are clamped.

Is my new comment explaining the code good enough? I tried keeping it short.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense! Yes, should be good to go now :)

Math.min(
currentLayerConfig.intensityRange[1],
currentLayerConfig.max != null ? currentLayerConfig.max : maximumInHistogramData,
),
];
} else {
newIntensityRange = [minimumInHistogramData, maximumInHistogramData];
}
yield* put(updateLayerSettingAction(layerName, "intensityRange", newIntensityRange));
// Here we also set the minium and maximum values for the intensity range that the user can enter.
// If values already exist, we skip this step.
if (layerConfigurations[layerName] == null || layerConfigurations[layerName].min == null) {
if (currentLayerConfig == null || currentLayerConfig.min == null) {
yield* put(updateLayerSettingAction(layerName, "min", minimumInHistogramData));
}
if (layerConfigurations[layerName] == null || layerConfigurations[layerName].max == null) {
if (currentLayerConfig == null || currentLayerConfig.max == null) {
yield* put(updateLayerSettingAction(layerName, "max", maximumInHistogramData));
}
}
Expand Down