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

Lifetime module branch #13

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

Mariochem92
Copy link

@Mariochem92 Mariochem92 commented Nov 1, 2023

Tackling issue #6 : This update facilitates phasor coordinate calculations for frequency-domain signals, conversion to phase, and effective single lifetime and mixture management. I am considering introducing spatial filtering and a couple of extra features but that also depends on the overall project structure you have in mind.Future enhancements, are open for community contributions, as I'm not well-versed in FRET or time-domain conversion.

@cgohlke cgohlke added the enhancement New feature or request label Nov 1, 2023
@cgohlke
Copy link
Contributor

cgohlke commented Nov 1, 2023

Hi @Mariochem92

thanks for contributing to this project.

Since this is the first contributor pull request, it may be a good time to review the contributor guide and get feedback on how to improve it. Should we create a PR template repeating the requirements regarding code and documentation style requirements?

There is certainly a large overlap with the code I started porting from the vLFD lifetime calculator mentioned in issue #6. That issue also included a FRET calculator, time-domain functions, and generating simple synthetic signals.

I am not sure how "n-dimensional aware" the code is. For example, ideally the functions would accept a scalar or sequence of laser frequencies but that may make the implementation too complex. Expressions like mod = (g**2 + s**2) ** 0.5 are memory intensive for images and could instead use numpy ufuncs if available, numpy.hypot in this case. Other cases like numpy.sqrt((1 - (g**2 + s**2)) / (omega**2 * (g**2 + s**2))) could be candidates for Cython implementations or refactored using inline expressions and replacing g**2 with g*g for example. Tests covering all cases are needed.

Since the phasorpy.io file readers are returning xarray.DataArray instances, it would be nice to get the metadata like laser frequency from the array object if possible. But that may be added in another PR.

I am not going to be able to do a full in-detail review for a while. @bruno-pannunzio and @schutyb

The module passes tests and frequency type is updated to float
@Mariochem92
Copy link
Author

Mariochem92 commented Nov 1, 2023

Hey @cgohlke thank you very much for clarification!

In #6 the code is mentioned but i am redirect to an image, do you have a valid link for the analysis script?

I updated the code so that it can pass the tests, but I will go through the very interesting points you raised in the next few days. I didn't get the n-dimensionality right before, thank you for explaining this through an example.

As for the contribution guide I think it's pretty well documented, it's my bad as I am not too acquainted with GitHub as of yet.

Also I was considering including spatial filters but I am not sure how those should be implemented, in my experience this is pretty intensive. Maybe Cython or scipy are the candidates there.

@Mariochem92
Copy link
Author

hey all! it looks like there are problems with imports from zenodo, but I haven't touched that part of the code. Not sure what's the problem there.

@cgohlke
Copy link
Contributor

cgohlke commented Nov 8, 2023

there are problems with imports from zenodo, but I haven't touched that part of the code. Not sure what's the problem there.

That sometimes happens with cloud services, We might have to look for alternatives if Zenodo turns out to be too slow or unreliable for running the tests.

@cgohlke
Copy link
Contributor

cgohlke commented Nov 9, 2023

Hi @Mariochem92.

@bruno-pannunzio, who is working on the FLIM part of this library, is defending his thesis these days. The Uruguayan team is also occupied with the 5th Workshop in Advanced Microscopy and Biophotonics on November 19-24. So, please allow some more weeks for the review of this PR. Bruno and I have existing code that overlaps with this PR and we want to make sure all comes together nicely.

To improve this PR, please add comprehensive unit tests to the tests directory and add the new module to the API documentation.

@Mariochem92
Copy link
Author

Mariochem92 commented Nov 13, 2023

No problem, best of luck to @bruno-pannunzio !

@bruno-pannunzio
Copy link
Contributor

bruno-pannunzio commented Jan 22, 2024

Hi @Mariochem92, sorry it took us so long to come back to you. We were discussing terminology, standardizing names and function signatures, so that we can all have some cohesion in the code we are developing. As you may see in the PR #28 , we have been discussing how to implement the manipulation of FLIM data, and more broadly, the phasor data. In the comments you can follow what we have agreed on and in the names and terminology we are going to be using, mainly:

signal: Time-domain, frequency-domain, or spectral data

decay, histogram: time-domain signal
wave: Frequency-domain signal
spectrum: Spectral signal
average (fdc): Average signal
background (fbg): Background signal
phasor: Phasor coordinates

real (re): Real component of phasor coordinates
imag (im): Imaginary component of phasor coordinates
polar: Polar coordinates

phase (phi): Angular component of polar coordinates in radians
modulation (mod): Radial component of polar coordinates
lifetime (tau): Fluorescence lifetime components

fraction (frac): Fractional intensity of lifetime component
amplitude (amp): Preexponential amplitude of lifetime component
apparent_lifetime Apparent single lifetime from phase or modulation

phase_lifetime (tauphi) Apparent single lifetime from phase
modulation_lifetime (taumod) Apparent single lifetime from modulation
reference: Signal, coordinates, lifetimes, etc used for calibration

real0 (re0): Real component of phasor coordinates for calibration
imag0 (im0): Imaginary component of phasor coordinates for calibration
phase0 (phi0): Angular component of polar coordinates for calibration
modulation0 (mod0): Radial component of polar coordinates for calibration
frequency: Laser or modulation frequency

omega or ohm: Radial frequency
omega_tau: Radial frequency multiplied with lifetime

Considering the code you proposed in your PR, I believe that there are some functions that can be partly covered by your code:

def phasor_to_polar(
    real: ArrayLike,
    imag: ArrayLike,
    /,
) -> tuple[NDArray[Any], NDArray[Any]]:
    """Return polar coordinates from phasor coordinates."""
    ...
    return phase, modulation

def phasor_from_polar(
    phase: ArrayLike,
    modulation: ArrayLike,
    /,
) -> tuple[NDArray[Any], NDArray[Any]]:
    """Return phasor coordinates from polar coordinates."""
    ...
    return real, imag

def phasor_from_lifetime(
    frequency: ArrayLike,
    lifetime: ArrayLike,
    /,
    fraction_or_amplitude: ArrayLike = 1.0,
    *,
    is_amplitude: bool = False,
) -> tuple[NDArray[Any], NDArray[Any]]:
    """Return phasor coordinates from lifetime distribution."""
    ...
    return real, imag


def phasor_from_apparent_lifetimes(
    phase_lifetime: ArrayLike,
    modulation_lifetime: ArrayLike,
    /,
    frequency: ArrayLike,
    *,
    frequency_axis: int | None = None,
) -> tuple[NDArray[Any], NDArray[Any]]:
    """Return phasor coordinates from apparent single lifetimes."""
    ...
    return real, imag


def phasor_to_apparent_lifetimes(
    real: ArrayLike,
    imag: ArrayLike,
    /,
    frequency: ArrayLike,
    *,
    frequency_axis: int | None = None,
) -> tuple[NDArray[Any], NDArray[Any]]:
    """Return apparent single lifetimes from phasor coordinates."""
    ...
    return tauphi, taumod

I think that this functions are partly covered by your implementation but needs thorough refactoring in order to meet the needs for this functions. Also you need to provide tests to cover all cases and documentation for all functions. You can read more in the contributor guide about tests and documentation required.

Please let us know what you think about our proposed standarized names and if you have any questions regarding this PR.

@bruno-pannunzio
Copy link
Contributor

Hi @Mariochem92, the PR #28 has been merged to main and the following functions listed before were already implemented:

  • phasor_to_polar
  • phasor_from_lifetime

The rest of the listed functions before are not implemented yet and can benefit from the code you already implemented that can be improved to meet the requirements of the library.

Please let us know if you have any question regarding this PR.

@cgohlke
Copy link
Contributor

cgohlke commented Mar 14, 2024

Hi @Mariochem92,

I think the following functionality from this PR has been implemented:

  • phasor_coordinates -> phasor_from_polar
  • phasemodule_values -> phasor_to_polar
  • lifetime_computation_phasor, lifetime_computation_array -> phasor_to_apparent_lifetime
  • refplot -> phasor_from_lifetime

fractional_intensities: not implemented yet.

phasor_lifetime: what is the advantage of this function over calling numpy.mean and numpy.std on phasor coordinates or apparent single lifetimes as needed?

apparent_lifetime: could you explain why this function returns the apparent single lifetimes from phase and modulation? As far as I understood, the phasor coordinates of the second, unknown, single component lifetime are on the universal circle, which means the apparent single lifetimes from phase and modulation should be identical. Yet, in your example, the values are slightly different. I am thinking to break this functionality down in phasor_from_lifetime, phasor_to_apparent_lifetime (both implemented), fractional_intensities, and semicircle_line_intersection (a specialized version of circle_line_intersection)

@cgohlke cgohlke added the awaiting feedback Waiting for reply label Mar 15, 2024
@Mariochem92
Copy link
Author

Hi @cgohlke ,
thank you for the extensive update. Maybe I got wrong what is meant by apparent single lifetime in this very context. I document the rationale behind my approach here. Feel free to break functionalities:)

@cgohlke cgohlke removed the awaiting feedback Waiting for reply label Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants