ome / omero-scripts

Core OMERO Scripts
https://pypi.org/project/omero-scripts/
12 stars 32 forks source link

Kymograph points string float #142

Closed will-moore closed 5 years ago

will-moore commented 6 years ago

This fixes the Kymograph script when Polylines are created with floating-point coordinates (as in iviewer). Also fixes the joining of sections in the new images for Polyline kymographs. To test, use image http://web-dev-merge.openmicroscopy.org/webclient/?show=image-138052 (user-3).

sbesson commented 6 years ago

A couple of high-level questions:

pwalczysko commented 6 years ago

For the follow-up workflow (Kymograph analysis) I get following output

screen shot 2018-08-14 at 11 38 35

Basically because I did not specify the timepoints, I end up with time in the units pixels and the speeds pixel per pixel. I understand where this is coming from, but shouldn't the Kymograph Analysis know about the pixels per timepoint and give me the speeds as pixels per frame ?

will-moore commented 6 years ago

This script should now work fine with the original unit16 image, uploaded to outreach at http://outreach.openmicroscopy.org/webclient/?show=image-28632. The script is also updated there and can be tested now.

The fixes above include the "direction" of the kymograph. As discussed with @pwalczysko: E.g. if I draw a line from A to B, the left-edge of the kymograph should correspond to A and the right-edge to B.

pwalczysko commented 6 years ago

Giving to a non-calibrated 5.12x512 image a calibration at the timepoint of the run of the script, namely 0.78 microns per pixel, and running the script, I end up with a Kymograph image which does seem to have a different pixel sizes in x and y directions, see http://outreach.openmicroscopy.org/webclient/?show=image-28639 and the screenshot below.

Is that logical/expected ? screen shot 2018-08-16 at 15 24 58

will-moore commented 6 years ago

@pwalczysko Yes, since the pixel size Y is time (seconds?) per pixel

pwalczysko commented 6 years ago

The Kymograph preserves the colors of the original images, but only for plain colors. For LUTs (and also for Greyscale), the Kymograph has the colors of the original image before changes of channel colors were appliead (=green and red)

@will-moore expected ?

pwalczysko commented 6 years ago

Other than that it seems to work nicely. Even the direction of the time from top to bottom is validated (I can see the particles in the image to go to the top right, which is preserved in the Kymographs).

Ready to merge FMPOV

joshmoore commented 6 years ago

See also "A couple of high-level questions" from https://github.com/ome/scripts/pull/142#issuecomment-412792009

will-moore commented 6 years ago

Re: https://github.com/ome/scripts/pull/142#issuecomment-412792009

will-moore commented 6 years ago

The other fix here, bringing get_line_data() into the script too, and changing it to work via the renderingEngine instead of getting pixel data is a much bigger change. This works well for Kymograph where we don't care too much about absolute pixel values, but we probably don't want that change elsewhere.

joshmoore commented 6 years ago

@will-moore : what of the changes in https://github.com/ome/scripts/pull/142#issuecomment-413816639 do you see getting into 5.4.8?

will-moore commented 6 years ago

If release is August 30th (https://trello.com/c/Xp8VUnOj/120-omero-548) there should be time enough to do all that, depending on any higher priorities? I'll create a card.

pwalczysko commented 6 years ago

I verified on eel that if you have the original image calibrated and timestamped, the values and speeds correctly propagate into the Kymograph, Kymograph Analysis and the resulting CSV.

On outreach, I have all data imported, but wonder now:

cc @will-moore @jburel

jburel commented 6 years ago

I think we were just going for workshops instead of the location The script won't be released

jburel commented 6 years ago

@will-moore test failure in Travis

pwalczysko commented 6 years ago

The script won't be released

Yes, so do we put it under workshops or under analysis (instead of the released script) on outreach ? @will-moore is in favor of the latter cc @jburel

joshmoore commented 6 years ago

I'm confused about the above. When there's some more time, a description of the workflow would be appreciated (and it's impact for this PR).

pwalczysko commented 6 years ago

Other than the question https://github.com/ome/scripts/pull/142#issuecomment-415392965, I have tested both the Kymograpph and Kymograph analysis. Both lines and polylines.

Works fine

Ready to merge FMPOV

pwalczysko commented 6 years ago

@joshmoore

a description of the workflow would be appreciated

Workflow might mean:

Which one ?

pwalczysko commented 6 years ago

@joshmoore For the whole Kymograph workflow, search for Kymograph in https://docs.google.com/document/d/11vnqqHTNSAi6pAtzfNy8d3-fy1VpFos0vR5kq2Mqmbw/edit#heading=h.f5zjcwbjevdi - the workflow is now ready to consume

jburel commented 6 years ago

Analysis is fine. 5.4.8 release is after the training so we will need to have the version used for Basel under ome/training-scripts as we have done previously and move it to this repo when it is ready for "official" inclusion Still some comments to address

Travis is still failing

joshmoore commented 6 years ago

My apologies, @pwalczysko, I was refering to PR/merging workflows.

will-moore commented 6 years ago

I opened https://github.com/ome/training-scripts/pull/37 to tag these fixes for the Basel workshop.

joshmoore commented 6 years ago

Thanks, @will-moore. Closing this for the moment then so it doesn't appear in the 5.4.8 builds.

will-moore commented 6 years ago

@joshmoore We'd still like to have this in 5.4.8. We're only porting it to training-scripts because we won't have 5.4.8 out before Basel.

joshmoore commented 6 years ago

i.e. the points from https://github.com/ome/scripts/pull/142#issuecomment-413816639 will be addressed this week?

will-moore commented 6 years ago

@joshmoore I already addressed those points (unless I missed something?) - I'll check...

will-moore commented 6 years ago

@joshmoore Currently the Kymograph scripts have their own copies of points_string_to_xy_list() so they can work with polylines containing floats with 5.4.7 server (as required for Basel workshop). Tests in the script repo have been updated to test this. I just opened a PR to port those changes back to the server: https://github.com/openmicroscopy/openmicroscopy/pull/5837 Which actually means that these scripts won't need their own fixes for that, once it is merged (hopefully into 5.4.8). However, the integration tests in the script repo using test-infra currently use OMERO 5.4.7, so we can't remove points_string_to_xy_list() fixes from the scripts in this PR as the tests will fail. Since this PR isn't urgent, I think we can wait for https://github.com/openmicroscopy/openmicroscopy/pull/5837 to get merged into 5.4.8 and then can remove points_string_to_xy_list() from the scripts and re-open this PR after the release.

get_line_data() in the Kymograph script now uses renderingEngine with current settings to get 8-bit data for the line, instead of getTile(), which is fine for Kymograph. I couldn't work out how to fix getTile() -> PIL for >8-bit data, so I don't have a fix get_line_data() in https://github.com/openmicroscopy/openmicroscopy/pull/5837.

will-moore commented 5 years ago

Recent training prep reminded us that this fix never made it into mainline (still need separate script to demo in Training workshops). cc @pwalczysko

will-moore commented 5 years ago

Test_infra testing of this script with OMERO 5.4.8 now passes without using a separate points_string_to_xy_list().

pwalczysko commented 5 years ago

Retested today again on

Both tests passed.

Ready to merge FMPOV

pwalczysko commented 5 years ago

Um, the only question was, shoiuld there not be a bump of a version advertised in the script ? see https://github.com/ome/scripts/pull/142/files#diff-a982bdd88ae79e1dbda8de7b27678ab5R33

will-moore commented 5 years ago

@pwalczysko Fixed the version to 5.5.

pwalczysko commented 5 years ago

Version fix works as expected. Ready to merge FMPOV.