ome / omero-parade

OMERO.web plugin for displaying Dataset thumbnails or Plates in webclient center panel
GNU Affero General Public License v3.0
1 stars 12 forks source link

Map ann support #27

Closed will-moore closed 6 years ago

will-moore commented 6 years ago

This allows filtering by Map Annotations.

NB: This PR is on top of @chris-allan python modules cleanup since I am editing one of the files that moved.

To test:

screen shot 2018-05-08 at 11 25 46

pwalczysko commented 6 years ago

After selecting a Key_Value in the menu (see screenshot) on the Dataset http://web-dev-merge.openmicroscopy.org/webclient/?show=dataset-18351 user-4

Traceback (most recent call last):

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/django/core/handlers/base.py", line 132, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)

  File "/opt/hudson/workspace/WEB-DEV-merge-deploy/OMERO.py/lib/python/omeroweb/decorators.py", line 488, in wrapped
    retval = f(request, *args, **kwargs)

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/omero_parade/views.py", line 127, in filter_script
    return module.get_script(request, filter_name, conn)

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/omero_parade/annotation_filters/omero_filters.py", line 199, in get_script
    'default': keys[0]},

IndexError: list index out of range

<WSGIRequest
path:/omero_parade/filters/script/Key_Value/,
GET:<QueryDict: {u'project': [u'1204'], u'_': [u'1525874543527']}>,
POST:<QueryDict: {}>,
...
pwalczysko commented 6 years ago

Default is the first key.

That is not what I see. On Dataset http://web-dev-merge.openmicroscopy.org/webclient/?show=dataset-19051 user-4, the first key is Temperature, but I get Pressure by default. screen shot 2018-05-09 at 15 28 13

screen shot 2018-05-09 at 15 28 08

Having one more look, my guess is that the key selected as default is the key which is alphabetically first. Not the first in the order of keys. That makes little sense, and probably was not intended like that.

pwalczysko commented 6 years ago

Another error, when I try to combine filter of Pressure from the comment with ROI count

screen shot 2018-05-09 at 15 32 35

Traceback (most recent call last):

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/django/core/handlers/base.py", line 132, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)

  File "/opt/hudson/workspace/WEB-DEV-merge-deploy/OMERO.py/lib/python/omeroweb/decorators.py", line 488, in wrapped
    retval = f(request, *args, **kwargs)

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/omero_parade/views.py", line 181, in get_data
    'histogram': list(histogram)

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/django/http/response.py", line 535, in __init__
    data = json.dumps(data, cls=encoder)

  File "/usr/lib64/python2.7/json/__init__.py", line 250, in dumps
    sort_keys=sort_keys, **kw).encode(obj)

  File "/usr/lib64/python2.7/json/encoder.py", line 207, in encode
    chunks = self.iterencode(o, _one_shot=True)

  File "/usr/lib64/python2.7/json/encoder.py", line 270, in iterencode
    return _iterencode(o, 0)

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/django/core/serializers/json.py", line 115, in default
    return super(DjangoJSONEncoder, self).default(o)

  File "/usr/lib64/python2.7/json/encoder.py", line 184, in default
    raise TypeError(repr(o) + " is not JSON serializable")

TypeError: 117 is not JSON serializable

<WSGIRequest
path:/omero_parade/data/Uk9JX2NvdW50/,
GET:<QueryDict: {u'project': [u'3001'], u'_': [u'1525874543564']}>,
POST:<QueryDict: {}>,
...
pwalczysko commented 6 years ago

Interestingly, this error I get when I select for non-zero parameters. First, I selected for Key_Value, then for sizeT. The dataset is http://web-dev-merge.openmicroscopy.org/webclient/?show=dataset-22351 the user is outreach on eel server. Note that all the images in the dataset have sizeT bigger than 2.

screen shot 2018-05-09 at 16 28 11

Traceback (most recent call last):

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/django/core/handlers/base.py", line 132, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)

  File "/opt/hudson/workspace/WEB-DEV-merge-deploy/OMERO.py/lib/python/omeroweb/decorators.py", line 488, in wrapped
    retval = f(request, *args, **kwargs)

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/omero_parade/views.py", line 181, in get_data
    'histogram': list(histogram)

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/django/http/response.py", line 535, in __init__
    data = json.dumps(data, cls=encoder)

  File "/usr/lib64/python2.7/json/__init__.py", line 250, in dumps
    sort_keys=sort_keys, **kw).encode(obj)

  File "/usr/lib64/python2.7/json/encoder.py", line 207, in encode
    chunks = self.iterencode(o, _one_shot=True)

  File "/usr/lib64/python2.7/json/encoder.py", line 270, in iterencode
    return _iterencode(o, 0)

  File "/home/hudson/virtualenv/lib/python2.7/site-packages/django/core/serializers/json.py", line 115, in default
    return super(DjangoJSONEncoder, self).default(o)

  File "/usr/lib64/python2.7/json/encoder.py", line 184, in default
    raise TypeError(repr(o) + " is not JSON serializable")

TypeError: 23 is not JSON serializable

<WSGIRequest
path:/omero_parade/data/c2l6ZVQ=/,
GET:<QueryDict: {u'project': [u'4451'], u'_': [u'1525877277050']}>,
POST:<QueryDict: {}>,
...
pwalczysko commented 6 years ago

I am in two minds here. Note, that none of the images in question has 0microM as value to their Key mitomycin-A. All have value 10microM. But my search for mitomycin-A, 0microM still delivers results, finding probably all ...0microM. This might need some thinking.

screen shot 2018-05-09 at 16 35 57

screen shot 2018-05-09 at 16 35 51

Edit: Thinking further about this, my values in the mitomycin-A Key include 10microM and 0mM, as well as 20mM. I think considering the above, we have an obvious case where the Values should be rather treated as numbers, not as strings, because I might get confusing search results. Would then the recommendation be to use the OMERO.tables instead in this case ?

pwalczysko commented 6 years ago

Several problems:

will-moore commented 6 years ago

Commit 1dca069 fixes the first error from https://github.com/ome/omero-parade/pull/27#issuecomment-387756855

The TypeError: 117 is not JSON serializable errors are probably due to web-dev-merge's numpy version (not related to this PR) but these are from the Add table data... menu which you don't need to test for this PR.

The ordering of keys is simply alphabetic, not related to the ordering of keys in the individual map annotations (see screenshot above). Since we could have a ton of images, each with different keys in different orders, I think it could be hard to use their order in the filter menu.

When filtering by query text, I simply test to see if the query is found in the Value, so it certainly works better for text values than numerical values.

pwalczysko commented 6 years ago

The ordering of keys is simply alphabetic, ...

Yes, well, it is an IT-alphabet. According to the screenshot, the capitalized Values are listed first in their entirety, then only come the non-capitalized ones starting with a. That is hardly natural, and would lead many users astray (oh, my Value is not in this list, because it is l which is between K and M, and I do not see it there). But understood about the impossibility to find a better deafault.

will-moore commented 6 years ago

@pwalczysko Thanks - should have remembered that Python sorts uppercase differently from lowercase. 7d68fc8 should fix that.

pwalczysko commented 6 years ago

Last commit (alphabetical order) works as expected, and it helps.

pwalczysko commented 6 years ago

That fix https://github.com/ome/omero-parade/commit/1dca0695b1e817da04993278876637e3c5d42019 does not work entirely. It prevents crash, but there is some weird behaviour.

The images vanish, the css is not great. Also, there is no dropdown on the Key_Value menu. See screenshot. screen shot 2018-05-11 at 11 45 37

http://web-dev-merge.openmicroscopy.org/webclient/?show=dataset-18351 user-4

After discussion with @will-moore he came up with the idea not to fileter unless a Key is really selected. In the csse described here, I really have no KVPs at all, but still, the filtering immediately kicked in, hiding all the images.

pwalczysko commented 6 years ago

RFE: Would it be possible to offer negative hits filetering ?

I mean following workflow:

temperature NOT 37

which would find that rogue image for me immediately ?

chris-allan commented 6 years ago

Will let these latest comments get addressed before merging.

joshmoore commented 6 years ago

which would find that rogue image for me immediately ?

I like the idea of this (and it annoys me when other sites don't provide this functionality) but this will likely have performance implications. Can I propose we should probably sit down next week and go over the types of queries that are likely to be needed and compare that with @mtbc's recent work to speed up map annotation queries?

pwalczysko commented 6 years ago

In summary

mtbc commented 6 years ago

Yes, my microservice should support queries like, "which images in these plates have temperature not 37?".

will-moore commented 6 years ago

That commit 538e924 adds a placeholder "Pick key..." which is shown initially and causes no filtering to occur. Only once you've chosen a key from the drop-down list does filtering happen. In the case of NO map annotations are found, you can't pick a key, so NO filtering happens.

will-moore commented 6 years ago

The "negative hits" would be useful for all filters, so this should really be handled by a "NOT" chooser that sits before each filter. It's just a UI design issue: how to make it look nice and understandable. But I'd prefer not to add this within various filters.

I found a slightly nicer workaround for the NOT 37 scenario, which is to filter (excludes 1 or 2 images), then select all filtered images (drag-select or select range) and now you can look at the complete image list in the jsTree and see which images are not selected. ;)

But this is obviously not very intuitive so a NOT operator would be handy.

will-moore commented 6 years ago

@joshmoore Here we are loading All map annotations for a given list of images (so we can do the filtering in the browser). I don't know that @mtbc's recent work will have much impact here, but if there's a faster way to it then that would be great.

pwalczysko commented 6 years ago

But I'd prefer not to add this within various filters.

That seems reasonable to me.

joshmoore commented 6 years ago

so we can do the filtering in the browser

Remembering the discussion we had during the Tuesday presentation, however, I'd warn against adding significant new filtering methods without planning through how we're going to scale it up.

jburel commented 6 years ago

Performance for large amount of data to handle is not covered in this PR (not its purpose) This is not only an issue for map annotation.

Pick key... is okay for now, this will be replaced by a spinner later on.

@will-moore will you create issue for the suggested RFE?

joshmoore commented 6 years ago

Performance for large amount of data to handle is not covered in this PR (not its purpose)

Definitely agreed, but new features should be discussed to be technically possible at scale nevertheless.

jburel commented 6 years ago

Each time a filer is removed the key_value pairs are reloaded and UI regenerated Example:

will-moore commented 6 years ago

That bug https://github.com/ome/omero-parade/pull/27#issuecomment-388823185 is not just for map_annotations, but will show up anytime you remove a filter (except for removing the last filter in the list). This is because the key that tells React whether each filter is a new vv existing component is based on the name and Index.

<ParadeFilter
    key={fname + idx}

So, React will re-load filters if the index changes, which includes re-loading the filter and data from OMERO. cc @chris-allan

will-moore commented 6 years ago

Kinda tricky to think how to fix that since you may have several filters with the same name e.g. Tag and a different Tag selected in each. If you remove one of them, we want React to remove just that one and not change the others. But it's hard to see how we can define a key for each filter based on the current state of each filter.

will-moore commented 6 years ago

Created https://github.com/ome/omero-parade/issues/38 from that last bug.

jburel commented 6 years ago

It is certainly more obvious now with the map annotation

jburel commented 6 years ago

Definitely agreed, but new features should be discussed to be technically possible at scale nevertheless.

Of course What I meant, is that we have various areas to consider. Thumbnails loading is a first one, the problem reported above is another one. The annotation query (not only map annotation) will be another one.