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.

Sign up for GitHub

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

Jump to bottom

Add function to project multi-harmonic phasor coordinates onto principal plane #78

Merged
merged 14 commits into from
Jul 23, 2024
Merged

Add function to project multi-harmonic phasor coordinates onto principal plane #78

merged 14 commits into from
Jul 23, 2024

Conversation

cgohlke
Copy link
Member

@cgohlke cgohlke commented Jul 1, 2024 •
edited
Loading

Description

This PR adds a single function, phasor_to_principal_plane, which projects multi-harmonic phasor coordinates onto the principal plane, along which coordinate axes the phasor coordinates have the largest variations. The function is implemented using PCA, finding the two most significant eigenvectors in the covariance matrix.

The function also tries to rotate and scale the projected coordinates such that their center lies in same quadrant and the projection of [1, 0] lies at [1, 0]. This allows for a more familiar presentation.

The transformation (projection, rotation, scaling) is affine. It preserves collinearity and ratios of distances, such that it should be possible to use the projected coordinates instead of phasor coordinates in some of PhasorPy's visualization and analysis functions.

This PR is work in progress. I am not confident that the single synthetic test is adequate. The affine transformation should also preserve convexity, but during testing I came across cases where that was not the case. Maybe there is still something wrong with the implementation or the method is numerically unstable...

See also doi:10.1021/acs.jpclett.0c02319. In the supplement it raises an interesting concern that (if I understood correctly) in FLIM data there is correlation in the frequency-dimension due to the presence of shot noise, which makes the projection more noisy.

Release note

Summarize the changes in the code block below to be included in the
release notes:

Add function to project multi-harmonic phasor coordinates onto principal plane

Checklist

  • The pull request title, summary, and description are concise.
  • Related issues are linked in the description.
  • New dependencies are explained.
  • The source code and documentation can be distributed under the MIT license.
  • The source code adheres to code standards.
  • New classes, functions, and features are thoroughly tested.
  • New, user-facing classes, functions, and features are documented.
  • New features are covered in tutorials.
  • No files other than source code, documentation, and project settings are added to the repository.

@cgohlke cgohlke added the enhancement New feature or request label Jul 1, 2024
@cgohlke cgohlke self-assigned this Jul 1, 2024
@cgohlke
Copy link
Member Author

cgohlke commented Jul 1, 2024

Apparently the synthetic test data lead to significant different results across platforms.

@cgohlke
Copy link
Member Author

cgohlke commented Jul 2, 2024

How about renaming the function to phasor_principal_components and add an ndims=2 parameter? It would allow, for example, to project the phasor coordinates onto 3 dimensions (are there any use cases?). However, it would make typing the returns, documentation, and the implementation of reorient more complex...

@cgohlke cgohlke changed the title WIP: Add function to project multi-harmonic phasor coordinates onto principal plane Add function to project multi-harmonic phasor coordinates onto principal plane Jul 11, 2024
@cgohlke
Copy link
Member Author

cgohlke commented Jul 11, 2024

I think this is ready for review.

@bruno-pannunzio
Copy link
Contributor

I think this is ready for review.

Excelent @cgohlke! I will have a look at this and #81 as soon as possible, as I need to finish a couple of things first of #48 so we can move forward with the PR.

bruno-pannunzio
bruno-pannunzio approved these changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@bruno-pannunzio bruno-pannunzio left a comment

Choose a reason for hiding this comment

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

@cgohlke I added some minor comments but I think this is ready to be merged. We should test with real data as you commented in the tutorial. Maybe with hyperspectral data, as FLIM data seems to be not good due to noise (I also understood the same from the paper).

src/phasorpy/phasor.py Show resolved Hide resolved
tutorials/phasorpy_principle_components.py Outdated Show resolved Hide resolved
@cgohlke
Copy link
Member Author

cgohlke commented Jul 22, 2024

Hi @bruno-pannunzio : Thanks for the review. I added the reference and corrected the spelling. Let's try to find a real dataset that could profit from this projection and apply the cursor visualization.

Do you think there could be any confusion from using the terms "components" (vs lifetime components) and "principal plane" (used in optics)?

How about renaming the function to phasor_principal_components and add an ndims=2 parameter? It would allow, for example, to project the phasor coordinates onto 3 dimensions (are there any use cases?). However, it would make typing the returns, documentation, and the implementation of reorient more complex...

Any thought about this?

@bruno-pannunzio
Copy link
Contributor

Oh sorry I didn't see the previous comment. I hadn't realized but you may be right that there could be some confusion with the lifetime components. That's why I think that although phasor_principal_components is more accurate, the current name may lead to less confusion. Maybe the tutorial title can be Principal Component Analysis, since PCA is a commonly known method and by adding the 'Analysis' it makes it much more clear that this is PCA and not the lifetime components analysis, or something like Projection to principal plane. Maybe @lmalacrida can give is some insight into this or we can discuss next meeting.

On the other hand, I don't think it will lead to confusion with the optical plane, since this is clearly an analysis and I am not aware of this kind of analysis related to the optical plane.

Related to the ndims, it would be nice maybe in the future to add the possibility to add more principal components, as it sometimes helps to include more dimensions (if they help to explain the variance of the data), but I am not aware if this is the case in the phasor analysis. I agree that it will add complexity to implementation and documentation and maybe other issues are more important at this stage.

@cgohlke
Copy link
Member Author

cgohlke commented Jul 22, 2024

Sounds good. Let's leave this PR open for now and briefly discuss tomorrow.

@bruno-pannunzio bruno-pannunzio self-requested a review July 22, 2024 19:41
@cgohlke
Copy link
Member Author

cgohlke commented Jul 23, 2024

OK, I am going to merge this even though it is an experimental/unproven feature.

@cgohlke cgohlke merged commit 9f8b46e into phasorpy:main Jul 23, 2024
16 checks passed
@cgohlke cgohlke deleted the pca branch July 23, 2024 19:47
schutyb pushed a commit to schutyb/phasorpy that referenced this pull request Aug 9, 2024
@cgohlke @schutyb
Add function to project multi-harmonic phasor coordinates onto princi…
1862249 
…pal plane (phasorpy#78)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schutyb schutyb Awaiting requested review from schutyb

@lmalacrida lmalacrida Awaiting requested review from lmalacrida

@bruno-pannunzio bruno-pannunzio Awaiting requested review from bruno-pannunzio

Assignees

@cgohlke cgohlke

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cgohlke @bruno-pannunzio