Skip to content

Conversation

@knabar
Copy link
Member

@knabar knabar commented Dec 1, 2012

No description provided.

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-merge-4.4#77. See the console output for more details.

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-merge-4.4#78. See the console output for more details.

@joshmoore
Copy link
Member

@knabar / @chris-allan, I've labelled this "Conflict" to shut snoopy up .

@knabar
Copy link
Member Author

knabar commented Dec 6, 2012

@joshmoore / @chris-allan I modified my webgateway/urls.py, so the conflict with PR 491 should be gone.

@joshmoore
Copy link
Member

Label removed.

@pwalczysko
Copy link
Member

Annotation visible in Full Viewer. ROIs visible as well. Works. Ready to merge.

@will-moore
Copy link
Member

It's possible to get this server error for the json call (not all annotations have namespace):

("error in call","Traceback (most recent call last):
  File "/Users/will/Desktop/OMERO/components/tools/OmeroWeb/omeroweb/../omeroweb/webgateway/views.py", line 1024, in wrap
    rv = f(request, *args, **kwargs)
  File "/Users/will/Desktop/OMERO/components/tools/OmeroWeb/omeroweb/../omeroweb/webgateway/views.py", line 1822, in object_table_query
    a = _annotations(request, objtype, objid, conn, **kwargs)
  File "/Users/will/Desktop/OMERO/components/tools/OmeroWeb/omeroweb/../omeroweb/webgateway/views.py", line 1740, in _annotations
    if annotation.getNs().val == NSBULKANNOTATIONS
AttributeError: 'NoneType' object has no attribute 'val'
")

@will-moore
Copy link
Member

I'm concerned about the mapping between the url and the iQuery construction. E.g. this works OK

webgateway/annotations/Plate/wells/817/

but this

webgateway/annotations/plate/wells/817/

gives me

jQuery172030647895121586133_1355141663023({"query": "select obj0 from plate obj0\njoin fetch obj0.wells obj1\n\n        join fetch obj0.annotationLinks links\n        join fetch links.child\n        where obj1.id=:id", "error": "[u'plate', u'wells'] cannot be queried"})

when I should really get a 404.

In fact, it seems strange that I need uppercase "Plate" but lowercase "wells".

Can't we simply specify the urls we need via the urls regex, or at least in the first few lines of views.py code, raising a 404 if we don't get something that is supported. We can also handle any conversion here if needed, "plate" -> "Plate" etc.

@will-moore
Copy link
Member

Hi Andreas, I wonder if you could try using 4 spaces instead of tabs in your html / javascript editing (you're doing this fine in Python). With a mix of spaces and tabs, the indentation is looking kinda strange, both in github's diff and in my TextMate, which makes it a little hard to work out what's going on.
Cheers,

@knabar
Copy link
Member Author

knabar commented Dec 10, 2012

Apologies for the tabs, I'll double-check my editor settings.

I'll add a check for annotations without namespaces.

The reason it's uppercase Plate but lowercase wells is that the code just takes the terms from the URL into the query. The first term is the object type to find (which must be uppercase), all following terms are names of properties of that object (lowercase in this example, but I guess could be anything, whatever the property is). What probably should happen is better error-checking and a 404 if the query resulting from the given URL is invalid.

@will-moore
Copy link
Member

@knabar Understood: I know this seems really picky, but I think the urls could be made a little cleaner:

  • Using a slash as the delimiter between Plate/wells but doing the objtype = objtype.strip('/').split('/') in views.py seems confusing, since you expect that slash-delimited tokens become separate parameters when passed to views.py. What about using 'Plate.wells'. E.g. webgateway/table/Plate.wells/1402/query/. This is easier to see that Plate.wells is a single objtype parameter.
  • Using the request 'query' directly in the tables service means that the urls look more complex than necessary, and are more 'fragile'. E.g. ?query=(Well%3D%3D1402). It might be nicer to support something like ?query=well-1402, which is what we do in other places in the webclient, E.g. webclient/?show=dataset-123.

@knabar
Copy link
Member Author

knabar commented Dec 13, 2012

@will-moore I made the suggested changes to PR 512

@will-moore
Copy link
Member

Great, thanks for that.

A couple of minor comments:

I would prefer the javascript within if statements to be indented, since it's easier to understand the scope of each block.

Also, if you happen to open an Image from a Plate with NO annotations on it, the json gives a 404 with

[u'Plate', u'wells'] with id 596 not found

even though Plate 596 does exist. This is because your query is relying on there being annotations.

If you added left outer to the join statements:

query += """
    left outer join fetch obj0.annotationLinks links
    left outer join fetch links.child
    where obj%d.id=:id""" % (len(objtype) - 1)

then you would still retrieve a Plate with no annotations and you would instead get the 'correct' message "Could not retrieve single bulk annotations table"

However, this is really splitting hairs because the outcome is the same, so not a blocker to merging.

@knabar
Copy link
Member Author

knabar commented Dec 13, 2012

@will-moore Sorry about the indentation issue, it happened when I replaced the tabs with spaces and I missed it.

I added the left outer joins, thanks for catching that. Misleading error messages are horrible.

@manics
Copy link
Member

manics commented Dec 14, 2012

The metadata isn't displayed if the populate_metadata.py script has been run twice on the same plate (works in Insight).

@will-moore
Copy link
Member

@knabar Those last commits are great.

One question: The test scenario that I was given by @pwalczysko included the display of bulk annotations when the table was attached to the Screen (as well as the Plate). But your code doesn't support this, right?

@manics's point about multiple bulk_annotations per Plate: I think you can just pick the most recent (highest ID) until we decide how to fully support this, E.g.

    if len(a['data']) < 1:
        return dict(error='Could not retrieve single bulk annotations table')

    fileId = max([ann['file'] for ann in a['data']])
    return _table_query(request, fileId, conn, **kwargs)

@jburel
Copy link
Member

jburel commented Dec 16, 2012

@knabar, @will-moore, @manics: All displayed in insight. I will check the order in place. I will probably sort by id in that case.

@knabar
Copy link
Member Author

knabar commented Dec 17, 2012

@will-moore Right now the Javascript only pulls in annotations for the plate into the image viewer. It should be easy to also query the screen and display annotations attached to it.

@will-moore
Copy link
Member

@knabar I think you'll need a custom query for "Screen.plate.well" since I don't think your current query-builder will parse this right. But I think that should be done in a different PR.

This current PR is ready to merge now.

@joshmoore
Copy link
Member

Ready to be rebased onto develop then, @knabar. Merging.

@joshmoore
Copy link
Member

@will-moore, @hflynn, @gusferguson, @pwalczysko, @jburel -- what's needed in the way of screenshots & docs?

joshmoore added a commit that referenced this pull request Dec 18, 2012
Display bulk annotations in web image viewer, new webgateway API calls
@joshmoore joshmoore merged commit d17a7d7 into ome:dev_4_4 Dec 18, 2012
@jburel
Copy link
Member

jburel commented Dec 18, 2012

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.

7 participants