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

Add support for space-time markers and LSM/FLIM raster measurements #45

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marktsuchida
Copy link

This follows discussion in #44. We agreed there that this should be merged after a v0.5 release, so I'm marking this PR as draft for now.

The first of 2 commits adds the fields for indicating detector ids used for "space-time markers", and to indicate which detector id is used for which FLIM/LSM functionality (i.e. pixel, line, or frame clock).

The second commit adds the fields to record measurement setup for raster scans (LSM), including FLIM. Compared to #44 (where we discussed fields /setup/flim_*), I have changed this to a subgroup /setup/raster, after noting that it can be used for any SPC LSM, not just TCSPC FLIM.

The two commits are orthogonal to each other.

Cc: @smXplorer and @talaurence.

@marktsuchida
Copy link
Author

Appreciate any comments or suggestions for change.
BTW Is there any plans about when v0.5 will be releasd?

@smXplorer
Copy link
Contributor

Appreciate any comments or suggestions for change.
BTW Is there any plans about when v0.5 will be releasd?

@marktsuchida Will look at this next week hopefully.

@@ -397,6 +407,60 @@ The following fields are also arrays:
This field is not relevant when no polarization- and spectral-insensitive
splitting is performed.

The following fields can be used to indicate the meaning of space-time markers.

- **num_space_time_markers**: (integer) *New in version 0.6.* number of
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should specify either this field is not included for non-space-time measurements, or that if there are no space-time markers, this field must be 0 if specified.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the following text already accomplish this? (The value must match the number of markers, namely zero, if there are no markers; and the field is not required in that case.)

measurements (typically FLIM, but could be any single-photon-counting
raster-scan imaging method).

These 3 fields are required if this group is present:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add a raster_type string under the setup/raster group so that we can leave open other non-raster scanning spatial modulations, each with its own set of maker to space-time conversion definitions?

We should specify if the raster_type is not raster then and additional field, maybe pixel_coordinates needs to be specified where the location of each pixel is located...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps space_time_markers_type or something, so that it's not called "raster" when it's not "raster"?

It does seem like a good idea to have this, even if we only add one type initially, for future extensibility.

(I don't have enough experience with non-raster scanning to know if recording a priori pixel coordinates is always possible or desirable (or even sufficient to recover an image). It may make sense for small scans, but I would expect for large scans one would want to record parameters for the curves instead.)

One less ambitious type that we might consider is bidirectional raster scans (in which alternating lines are scanned in opposite directions), though I don't feel this is important enough to put into an initial version.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the naming space_type_markers_type is better than raster.

I think specifying a new type or other way of defining bidirectional raster scans would be a good addition as well, and not very difficult.

I'll say that I agree that recording apriori pixel coordinates is not the most efficient, and not necessarily ideal, my motivation was to provide a poor, but still sufficient way of recording the location when someone uses a non-standard format.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yet, bi-directional is not as simple as it seems. The frame and pixel clock (if present) need not change, but the line clock (if present) could be a single clock every line or every other line, or two separate clocks for each direction. Also, at least in theory the first line could be left-to-right or right-to-left. And if two separate clocks are used, the direction of the first line could vary from frame to frame (again, at least in theory).

But this might be a good illustration of the situation we face: if we try to create a spec that covers all these possibilities, it will be complicated and hard to understand (and use correctly), it may include features that are theoretical and never end up being used, and after all that, we could easily miss some possibility (or a new variant may be invented in the future).

I think it is much more important to design for incremental changes, so that each space-time-markers-type (if we decide to go with this way) is as simple as possible, and new ones can be added with less work. Perhaps this means having more than one type for bi-directional (rectangular) rasters to account for the different patterns. That way, over time, we can add the patterns that are actually encountered (or proposed) in hardware.

In the bi-directional scanning case, for example, we could add a markers-type for the case of a single line clock recorded for every line. Support for any other pattern can later be added if/when we actually find the need, either as a a different markers-type or by updating the spec for the markers-type-specific fields.

(Personally I think it is better to get out a new release of the spec with the minimal and by far most common case of uniform, unidirectional, rectangular raster scans, leaving the addition of support for the other possibilities for subsequent releases.)


- **raster_width**: (integer) width of raster scan (in pixels)
- **raster_height**: (integer) height of raster scan (in pixels)
- **pixel_time**: (float) the pixel period of the raster scan (in seconds)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How necessary is this (and should it be required) as this information could theoretically be inferred from the marker timestamps, and could there be a FLIM imaging mode where different pixels have different times? I.e. maybe a setup where the acquisition software waits on a pixel until a certain number of photons are collected instead of a specific time to move onto the next pixel.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raster scans can be recorded with any of (frame, line), (frame, line, pixel), (line, pixel), (line,), or (pixel,) clocks/markers (and theoretically other combinations). I have seen most of these in actual use (not all scanners produce all 3 clocks, and on some TCSPC devices (Swabian) it helps to conserve marker channels). If you only have a line clock, for example, you need all 3 of these fields to recover an image. It is also possible to have a scanner that produces a constant pixel clock even during retrace, so that you need to count the pixels following each line marker.

Some of these could probably be made optional -- especially the pixel_time when a pixel clock is being recorded. I do think it is easier to record at least the width and height, though, because processing code will be more complex if it has to determine these as it goes (requires reallocating the image buffer or reading the data twice). (A minor point is that there is also the case where a partial frame is recorded at the end of the stream, and if the first frame happens to be partial, there is no way to determine how many lines there were supposed to be.)

I don't think waiting at a pixel for a photon count is very practical (with software in the loop, at least, due to high latency), but a resonant scanner is a case where a single pixel_time field won't work. My stance in writing this was that we don't (yet) support the case of resonant scanners. Maybe that could be added in the future as a raster_type other than raster.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I wasn't aware that different scans didn't record "all" markers.

In that case, then yes, it makes sense to require recording width/height/time markers.

@marktsuchida
Copy link
Author

Transplanting @smXplorer's comment:

Not sure whether assuming that horizontal calibrations are identical in X and Y is general enough?
As a matter of fact, I encountered a case of a scanner that scanned at an angle... that is where the X and Y directions where not orthogonal.
In other words, one may potentially want to provide means to specify each direction individually (in terms of spherical angles and step size? but what about resonant scanner where this notion might not even be applicable?).

I agree, but as with the case of bi-directional scanning (see above), I don't know if this is essential in the immediate next release. Not opposed if a very simple and unambiguous spec can be created.

Resonant scanners could record the H-sync (line clock) in at least a few ways, and the metadata needed to express the required linearization probably needs some work to get right. I'm interested in this, since as of recent we have one in the lab, but I also think it would be okay to leave this for later. I think for this we likely want to use a different overall "type" (as opposed to the default raster).

@harripd
Copy link

harripd commented Jul 21, 2024

I agree, but as with the case of bi-directional scanning (see above), I don't know if this is essential in the immediate next release. Not opposed if a very simple and unambiguous spec can be created.
...
I think for this we likely want to use a different overall "type" (as opposed to the default raster).

I think I agree about the next release not needing to support the full range of scan types, but we do want to include a field for the type of scanning, so we have extensibility in the future.

To the group for space-time modulation, add a string scan_type of which in the current spec we will provide limited options.

  • "raster" defined as @marktsuchida has defined
  • "generic" the space-time modulation group that contains two arrays (comments welcome), this group's purpose is to provide a way for any modulation to be implemented, but is inefficient, as each position and time of movement is recorded.
    • time_divisions (integer 1D array) the times when the scanner moves, in the clock rate of the experiment
    • positions (float 3xN array) the xyz positions of each "pixel"
  • bidirectional (least likely to be implemented in this version) like raster, but accounts for differences in even and odd lines etc. as per @marktsuchida 's comments

@harripd
Copy link

harripd commented Jul 21, 2024

Also, as a quick thought, if we are moving towards having future support for other scan methods, maybe instead of space_time and raster maybe we rename one or both of these to scan as an easier to understand word for those reading this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants