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

implemented get_timestamps() from timeIncrement attribute #589

Merged
merged 7 commits into from
Oct 3, 2024

Conversation

JensWendt
Copy link
Contributor

@JensWendt JensWendt commented Sep 17, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

JensWendt commented Sep 19, 2024

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

EDIT:
Fixed. we can go ahead.

@JensWendt
Copy link
Contributor Author

-.-
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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@JensWendt
Copy link
Contributor Author

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

@will-moore
Copy link
Member

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

@will-moore will-moore merged commit d07375a into ome:master Oct 3, 2024
1 check passed
@will-moore will-moore added this to the 7.2.0 milestone Oct 3, 2024
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.

2 participants