ome / omero-figure

An OMERO.web app for creating Figures from images in OMERO
http://figure.openmicroscopy.org
GNU Affero General Public License v3.0
15 stars 31 forks source link

implemented get_timestamps() from timeIncrement attribute #589

Closed JensWendt closed 1 month ago

JensWendt commented 2 months ago

Changed get_timestamps() to revert back to the timeIncrement attribute of Pixels of an Image, if no PlaneInfo is found for it, or if the PlaneInfo did not contain DeltaT information. If no timeIncrement attribute is found, it will just return 0 for all timepoints. This does not account for any time offset that might have occured, i.e. the first image always is at 0s.

will-moore commented 2 months ago

Thanks @JensWendt. If there's no timeIncrement, can we just keep the existing behaviour of returning an empty list? Then we know that an empty list means "No timestamps", rather than a list where all the values are 0.

JensWendt commented 2 months ago

Done! now it checks if time_increment != 0 and leaves the dict empty if not, which in turn leaves the list empty again, as it was before.

JensWendt commented 2 months ago

Hold! no merging yet, please. I found a bug.

EDIT: Fixed. we can go ahead.

JensWendt commented 2 months ago

-.- now the timeIncrements are converted to SECOND BUT I cannot open images, where I manually set the timeIncrement with an OMERO.web script, anymore in Figure --> Error Image not found on the server, or you don't have permission to access it at /figure/imgData/584306/ It is my image, nobody else touched it. I could open it just fine before the latest "fix" why is converting the time somehow preventing me from opening the image?

will-moore commented 2 months ago

@JensWendt Are you sure you're still logged in as the same user (your connection hasn't timed out and you're now logged-in as e.g. public user)? Can you still access your other images?

JensWendt commented 2 months ago

nevermind, I simply did not add the new imports in the live omeroutils.py file on our server, only updated the function. I assume somehow this broke the import of the image as soon as TimeI got called, because I could still open Images that were going the route of PlaneInfo. For me everything works now, it should be ready.

will-moore commented 2 months ago

Hi @JensWendt I set timeIncrement based on your code at https://forum.image.sc/t/how-to-correctly-set-a-timeincrement-for-omero-pixels/101600

But the conversion of the time into seconds in this PR wasn't working for me. (are you sure it's working for you?) This is the change I needed to get this working...

--- a/omero_figure/omeroutils.py
+++ b/omero_figure/omeroutils.py
@@ -48,9 +48,9 @@ def get_timestamps(conn, image):
         try:
             pixels = image.getPrimaryPixels()._obj
             time_increment = pixels.getTimeIncrement()
-            time_increment_unit = time_increment.getUnit()
-            converted_value = TimeI.CONVERSIONS[time_increment_unit][
-                UnitsTime.SECOND](time_increment._value)
+            secs_unit = getattr(UnitsTime, "SECOND")
+            seconds = TimeI(time_increment, secs_unit)
+            converted_value = seconds.getValue()

         except Exception as error:
JensWendt commented 2 months ago

Hi,

I just double checked, and yes, it does work on my machine :)

But your solutions seems a lot nicer and more intuitive, if I would have known it works like this I would have done it that way. I think I missed that Constructor because I looked at Time instead of TimeI -.- Once again I am lost in the jungle of OMERO documentation

will-moore commented 2 months ago

Ah, OK I realise what's going on. You can't convert from a 'SECOND' to a 'SECOND'! So if your timeIncrement is already in SECONDS then it fails.

This works OK...

>>> TimeI.CONVERSIONS[UnitsTime.SECOND][UnitsTime.MILLISECOND]
<omero.conversions.Mul object at 0x11007fd30>

Bun this fails (which is the error I was seeing).

>>> TimeI.CONVERSIONS[UnitsTime.SECOND][UnitsTime.SECOND]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: SECOND

So I guess your timeIncrement was not in SECONDS?

I agree that this is confusing and docs are lacking, so I think you actually did pretty well! I just happened to remember relevant code at https://github.com/ome/omero-py/blob/4fd72efaa8cdbec9da98e1d28678af16878a86ba/src/omero/gateway/__init__.py#L292 and adapted that.

JensWendt commented 2 months ago

Yes, I tried with minutes and hours, because that was what initially didnt work after my first iteration.

JensWendt commented 2 months ago

did a commit with your solution. Tested it with millisecond, second, minute and it works now as expected/intended

will-moore commented 1 month ago

This is working fine for me now - tested on merge-ci server. Merging...