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

histograms #4703

Merged
merged 50 commits into from
Dec 14, 2016
Merged

histograms #4703

merged 50 commits into from
Dec 14, 2016

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Jun 9, 2016

This adds histograms to Preview panel of web (will also add to image viewer - not ready yet).
Based on designs at ome/design#37
Also https://trello.com/c/6gKn8FT2/40-histogram

To test (NB: start with refresh of browser to avoid cached js files)

  • Open image in Preview Tab
  • Click "Show Histogram"
  • Need full test of editing rendering settings (few code changes in this)
  • Histogram should show channel being edited (slider/colour-picker/toggle)
  • Lines on histogram showing start/end of channel sliders should be shown on histogram and updated when adjusting sliders.
  • Also test histogram updates to 'current channel' and start/end lines in sync with slider values during Save, Save To All, Undo / Redo, Copy / Paste, 'Imported' settings, greyscale etc.
  • Test similar in full image viewer.

screen shot 2016-06-09 at 23 22 52

NB: I have removed the 'rangewidget' which gives channel start/end inputs their up/down buttons. I think this wasn't useful functionality and looks better with it gone. See before (above) and after (below)

screen shot 2016-06-12 at 23 59 21

@joshmoore
Copy link
Member

Think a file is missing:

https://cowfish.openmicroscopy.org/webmerge/static/webgateway/js/ome.histogram.js Failed to load resource: the server responded with a status of 404 (Not Found)

We try to only trigger channel changes when a channel changes and if several channels
change E.g. setModel(greyscale) then the active channel is triggered last.
Also have tried not to use rgb colours anywhere since the coversion between rgb
and hex colours is unecessary and we don't want to have to handle both when
channel change is triggered
@@ -113,6 +115,7 @@
<script type="text/javascript" src="{% static "3rdparty/aop-1.3.js" %}"></script>
<script type="text/javascript" src="{% static "webgateway/js/ome.postit.js"|add:url_suffix %}"></script>
<script type="text/javascript" src="{% static "3rdparty/farbtastic-1.2/farbtastic.js" %}"></script>
<script type="text/javascript" src="{% static "webgateway/js/ome.histogram.js" %}"></script>
Copy link
Member

Choose a reason for hiding this comment

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

you are missing url_suffix

Not much use to be able to click up and down a single value. Also made the UI more
cluttered.
@will-moore will-moore closed this Jun 13, 2016
@will-moore will-moore reopened this Jun 13, 2016
@pwalczysko
Copy link
Member

pwalczysko commented Jun 14, 2016

Behaviour:

  • turn one channel off
  • then observe that the turned off channel is still displayed on histogram
  • there is a discrepancy now between the preview image and the histogram shown
  • suggest: do get in sync when channel are being turned off with the Preview, if a channel is turned off, show the next channel which is ON in the histogram
  • when the user starts adjusting the bars of a channel which is OFF, put the histogram of that channel on display (this is a discrepancy with the preview image, but directly user-caused one)

Edit:

  • just realized: when a channel is turned on, the histogram should show that channel (this is atm actually the only way how to switch between channels in histogram without moving the bars !)

@pwalczysko
Copy link
Member

RFE:

  • the bar in the histogram on the right-hand side tends to completely disappear as I drag the right slider to the right, this happens quite readily even when I am at Min/Max settings
  • I understand that the Histogram is meant to show just the "relevant" part of the range, but what is it actually showing then when not Min and Max ?

@pwalczysko
Copy link
Member

RFE:

  • be on second channel (second channel is selected in Histogram)
  • click "Imported"
  • observe that the "Imported" Rnd settings have been indeed applied, but
  • there is a third channel now displayed in Histogram (the last one on the image)
  • I think the channel selection should not change if I apply "Imported" (or anything else indeed), as there is a clear "deal" that the Histogram channel changes only when I drag the bars or switch the channel on

@pwalczysko
Copy link
Member

Bug: Unsolicited switching of channels in histogram after first open.

  • In Preview panel, navigate to image https://cowfish.openmicroscopy.org/webmerge/webclient/?show=image-21920
  • this image has two channels, green and yellow
  • start dragging the green right bar to the left, let it go
  • observe that the Histogram became yellow -> not expected, the yelow channel was not touched nor was it switched on or off (both channels are still on as they were when you opened the image)

@pwalczysko
Copy link
Member

Couple of RFEs and one bug. Stopping testing for today.

@gusferguson
Copy link

Issues with default channel shown by histogram:

  • Opens - shows top channel by default - leave unchanged or change any channel - click Imported - histogram changes to bottom channel. Ditto for the Min/Max and Full Range buttons.
  • As user-3 went to image belonging to user-5 which had been changed by owner - adjusted top channel using slider - on release of slider histogram jumped to bottom channel.
  • Saved settings as user-3 - clicked on user-5 settings - histogram showed bottom channel by default. Ditto when clicking back onto user-3 settings.

Selecting slider on another channel or clicking into text box does not change histogram - have to change value to trigger - no way to just look at the histogram of a channel as it is without having to change something and then change it back.

If I decrease the minimum value in a channel or increase the maximum value then there is no change to range displayed in the histogram

The font size of the values displayed on the histogram needs to be reduced - too intrusive.

Find it very confusing only having the max value text on the histogram only appearing as the line moves to the left. Only having the min value displayed by default just triggers “what is that number?”. Should have the max value to the left of the max line, not to the right. I think it may also be better to have the text colour the same as the channel and line as this will make context more obvious.

If I increase the max value beyond the default using the text box, then use the slider to reduce it and - without releasing slider - go back to max - the value decreases. Releasing the slider and then repeating this - the max value continues to decrease progressively until the default maximum is reached and then it seems to stablise on that. I think this may have something to do with the slider still taking in values after the cursor on the slider has gone beyond the max point of the slider. In Insight the moment you click on the max slider button the max value returns to the default value. Min slider button/values behaves the same as max in both. This does occur in 5.2.3 but is intermittent.

@gusferguson
Copy link

@will-moore @pwalczysko
Another thought - I would prefer the Show histogram and Greyscale checkboxes to be swapped - makes Histogram more visible - will be used more than the Greyscale checkbox - and puts Greyscale over colour pickers - which is a bit better logic/association.

Don't set histogram to different channel on 'channelChange' since this is triggered multiple
times when settings are pasted/reset etc. Instead, trigger specific 'channelToggle' when
channels are toggled on/off
@will-moore
Copy link
Member Author

will-moore commented Jun 16, 2016

Should be mostly fixed now.

  • Histogram channel changes when you Toggle a channel (if toggle off, histogram switches to different channel)
  • Also changes channel when you click on the text start/stop inputs or drag slider.
  • Doesn't change channel when you Paste/Reset/Undo/MinMax etc.
  • Changes to the enabled channel when greyscale enabled and when channels toggled.
  • I removed the numbers on histogram markers as they were redundant and confusing.
  • Fixed the disappearance of histogram markers going off the right of the chart.

Other things I forgot to mention before:

  • Test that histogram updates when you change Z/T sliders.
  • Also check that histogram can't be turned on for Big images.

@gusferguson
Copy link

gusferguson commented Jun 17, 2016

Better without the numbers in the histogram.

Toggling channel works as expected.
Not defaulting to bottom channel when resetting to imported/min/max etc. - fixed.
Greyscale selection works as expected.
Changes with Z and T change works as expected.
Disabled with big images works as expected.

Histogram not changing colour when selecting a slider on another channel - only when moving - is this because selecting a slider without moving it does nor trigger an event?
Does change when clicking into range text box.

Like @pwalczysko - finding the behaviour between full range/min/max confusing. Histogram only shows between min and max and lines only appear on histogram when min/max is reached.
It is logical but the behaviour is not transparent.
Not sure if possible/worth trying to make it easier to grasp.

Would still like histogram and greyscale boxes swapped sides.

@will-moore
Copy link
Member Author

TODO: remove rendering-engine code for histograms of projected images.
Either disable in the UI when projection enabled or temporarily use non-projected histogram.

@will-moore will-moore mentioned this pull request Dec 1, 2016
@will-moore
Copy link
Member Author

will-moore commented Dec 1, 2016

  • Removed rendering-engine based code for histograms of Z-projection images.
  • Fixed 'problem' with greyscale flipping of histogram channel reported above histograms #4703 (comment)
  • Added ?bins=100 parameter to the json histogram. Test with E.g. webgateway/histogram_json/IMAGEID/channel/0/?bins=10 or remove bins parameter to get 256 bins.
  • Added histogram test to test this. Should be green.

@will-moore will-moore closed this Dec 1, 2016
@will-moore will-moore reopened this Dec 1, 2016
@will-moore will-moore closed this Dec 1, 2016
@will-moore will-moore reopened this Dec 1, 2016
@will-moore will-moore closed this Dec 1, 2016
@will-moore will-moore reopened this Dec 1, 2016
@gusferguson
Copy link

@will-moore

Tested using http://web-dev-merge.openmicroscopy.org/webclient/ user-3

Behaves as expected.
Works with Z-projected images.
Greyscale issue appears to be fixed.
Did not repeat the comparison with Fiji - @pwalczysko to do that if required.

Good to merge from my POV.

@@ -1570,7 +1570,11 @@ def load_metadata_preview(request, c_type, c_id, conn=None, share_id=None,
'c': ",".join(chs),
'm': r['model'] == 'greyscale' and 'g' or 'c'
})
maxW, maxH = conn.getMaxPlaneSize()
Copy link
Member

Choose a reason for hiding this comment

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

could you for the new variables use max_w etc.?

@pytest.mark.parametrize("bins", [None, 10])
def test_histogram_bin_count(self, bins):
"""
Test that the we get histogram json of the expected size.
Copy link
Member

Choose a reason for hiding this comment

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

typo

size_x = 125
size_y = 125
img_id = self.create_test_image(size_x=size_x, size_y=size_y,
session=self.sf).id.val
Copy link
Member

Choose a reason for hiding this comment

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

getId().getValue() is preferrable

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm - why is .getValue() preferable?
We use .val a lot everywhere. E.g.

OmeroWeb wmoore$ grep ".val" -r test/integration/ | wc
     727    3229   64263

Copy link
Member

Choose a reason for hiding this comment

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

In the Python stack we almost universally prefer the id.val style rather than using the accessor methods.

Copy link
Member

Choose a reason for hiding this comment

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

understood so will need to go over the inconsistency I have noticed in various locations
The style probably evolves through time

"""
Gets a histogram of 256 columns (grey levels) for the chosen
channel of an image. A single plane is specified by ?theT=1&theZ=2
or use ?p=intmax for a projection.
Copy link
Member

Choose a reason for hiding this comment

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

this is no longer the case
no pintmax

@waxenegger
Copy link
Member

Looks all good, tested on eel as well as locally.

@jburel jburel merged commit 359263e into ome:develop Dec 14, 2016
@jburel jburel added this to the 5.3.0 milestone Mar 29, 2017
@will-moore will-moore deleted the histograms branch February 18, 2019 04:09
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.

None yet