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

WIP: multi components analysis #106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

WIP: multi components analysis #106

wants to merge 1 commit into from

Conversation

schutyb
Copy link
Contributor

@schutyb schutyb commented Sep 1, 2024

Description

...

Release note

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

...

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.

@schutyb schutyb added the enhancement New feature or request label Sep 1, 2024
@schutyb
Copy link
Contributor Author

schutyb commented Sep 1, 2024 •
edited
Loading

Hi @bruno-pannunzio I added this functions as we were talking before. I need some help:

1 - The data is in another zenodo so I don´t know how to import it from that.
2 - Try the example in the tutorial with the data Alex gave to us, and you will see the result for each fractions are inconsistentes, there can be some negative values that corresponds to those components that have much noise, but not in the entire data.

Have a look whenever you can and let me know later.

@cgohlke
Copy link
Member

cgohlke commented Sep 1, 2024

data is in another zenodo
the data Alex gave to us

Where can we find those data?

cgohlke
cgohlke reviewed Sep 1, 2024
View reviewed changes
src/phasorpy/components.py Outdated Show resolved Hide resolved
src/phasorpy/components.py Outdated
Comment on lines 325 to 337
Parametres
----------
multi_harmonic_real : array_like
Real components of the phasor coordinate for many harmonics.
multi_harmonic_imag : array_like
Imaginary components of the phasor coordinate for many harmonics.
matrixA : array_like
Coefficiency matrix for each components.

Returns
-------
fractions : ndarray
Fractions of each components.
Copy link
Member

Choose a reason for hiding this comment

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

The documentation needs to be much expanded and more concise. What are the shapes/dimensions of the input and output arrays and how are they related?

src/phasorpy/components.py Outdated
Comment on lines 345 to 397
Examples
--------
>>> component_unmixing_from_phasor(
... multi_harmonic_real=[-0.4129963 , -0.07156281],
... multi_harmonic_imag=[ 0.44520192, -0.17987708],
... matrixA=[[-0.62442218, -0.47591901, -0.15022351, -0.5899417 ],
... [ 0.10858431, -0.0977382 , -0.24658903, 0.02135662],
... [ 0.21045545, 0.53110438, 0.61459059, 0.41559306],
... [-0.19656654, -0.3379749 , -0.06848862, -0.29857087],
... [ 1. , 1. , 1. , 1. ]]
... ) # doctest: +NUMBER
(array([0.3, 0.1, 0.4, 0.2]))

>>> multi_harmonic_real =
... [[[-0.39595987, -0.08619137],
... [-0.39595987, -0.08619137],
... [-0.39595987, -0.08619137]],
... [[-0.39595987, -0.08619137],
... [-0.39595987, -0.08619137],
... [-0.39595987, -0.08619137]],
... [[-0.39595987, -0.08619137],
... [-0.39595987, -0.08619137],
... [-0.39595987, -0.08619137]]]

>>> multi_harmonic_imag =
... [[[ 0.45231211, -0.16960909],
... [ 0.45231211, -0.16960909],
... [ 0.45231211, -0.16960909]],
... [[ 0.45231211, -0.16960909],
... [ 0.45231211, -0.16960909],
... [ 0.45231211, -0.16960909]],
... [[ 0.45231211, -0.16960909],
... [ 0.45231211, -0.16960909],
... [ 0.45231211, -0.16960909]]]
>>> matrixA=[[-0.62442218, -0.47591901, -0.15022351, -0.5899417 ],
... [ 0.10858431, -0.0977382 , -0.24658903, 0.02135662],
... [ 0.21045545, 0.53110438, 0.61459059, 0.41559306],
... [-0.19656654, -0.3379749 , -0.06848862, -0.29857087],
... [ 1. , 1. , 1. , 1. ]]

>>> component_unmixing_from_phasor(
... multi_harmonic_real,
... multi_harmonic_imag,
... matrixA) # doctest: +NUMBER
array([[[0.32982308, 0.11319236, 0.41102329, 0.14604279],
... [0.32982308, 0.11319236, 0.41102329, 0.14604279],
... [0.32982308, 0.11319236, 0.41102329, 0.14604279]],
... [[0.32982308, 0.11319236, 0.41102329, 0.14604279],
... [0.32982308, 0.11319236, 0.41102329, 0.14604279],
... [0.32982308, 0.11319236, 0.41102329, 0.14604279]],
... [[0.32982308, 0.11319236, 0.41102329, 0.14604279],
... [0.32982308, 0.11319236, 0.41102329, 0.14604279],
... [0.32982308, 0.11319236, 0.41102329, 0.14604279]]])
Copy link
Member

Choose a reason for hiding this comment

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

The examples are too complicated. Move them to the unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @cgohlke ok with this just to make sure, I will move it to the unit test, but shoul I leave the example empty here?

Copy link
Member

Choose a reason for hiding this comment

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

The first example can probably be simplified. Drop the named arguments and shorten the values.

src/phasorpy/components.py Outdated Show resolved Hide resolved
src/phasorpy/components.py Show resolved Hide resolved
src/phasorpy/components.py Outdated
ValueError
If multi_harmonic_real and multi_harmonic_imag have different shape.
If coefficiency matrix is empty.

Copy link
Member

Choose a reason for hiding this comment

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

Add a reference to publication :ref:Vallmitjana et al 2022 <vallmitjana-2022>?

src/phasorpy/components.py Outdated
fractions[r, c] = numpy.linalg.lstsq(
matrixA, vecB, rcond=None
)[0]
return fractions
Copy link
Member

Choose a reason for hiding this comment

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

The function is not tested.

tutorials/api/phasorpy_components.py Outdated Show resolved Hide resolved
tutorials/api/phasorpy_components.py Outdated
# Components unmixing from phasor
# -------------------------------
#
# Multi components unmixing from phasor.
Copy link
Member

Choose a reason for hiding this comment

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

Add a short description of the method and dataset.

src/phasorpy/components.py Outdated Show resolved Hide resolved
@cgohlke cgohlke mentioned this pull request Sep 3, 2024
Merged
9 tasks
@cgohlke
Copy link
Member

cgohlke commented Sep 3, 2024

Do I understand correctly that the harmonics are expected in the last dimensions of the phasor coordinates?

When implementing phasor_from_signal function, we settled that harmonics, if any, are in the first dimension. Ideally, this is handled with an axis parameter but that would complicate the implementation of several functions. Anyway, the default for harmonics should be axis=0.

@schutyb
Copy link
Contributor Author

schutyb commented Sep 3, 2024 •
edited
Loading

Do I understand correctly that the harmonics are expected in the last dimensions of the phasor coordinates?

When implementing phasor_from_signal function, we settled that harmonics, if any, are in the first dimension. Ideally, this is handled with an axis parameter but that would complicate the implementation of several functions. Anyway, the default for harmonics should be axis=0.

Yes, because when using the numpy.linalg.lstsq(a, b) https://numpy.org/doc/stable/reference/generated/numpy.linalg.lstsq.html function I pass parametrer b, with the real and imag part for each hamonic there, that is the way I found to relate coefficients in the parametres a and b in lstsq, there maybe a better way. I did this because we need to apply lstsq for each (real, imag) pair in the image.

I will update the PR soon when I finish your review and corrections. And we can find a better way to solve the linear eqaution,
do you know or suggets another alternative to numpy.linalg.lstsq(a, b)?

Thanks

@schutyb
Copy link
Contributor Author

schutyb commented Sep 3, 2024

Do I understand correctly that the harmonics are expected in the last dimensions of the phasor coordinates?
When implementing phasor_from_signal function, we settled that harmonics, if any, are in the first dimension. Ideally, this is handled with an axis parameter but that would complicate the implementation of several functions. Anyway, the default for harmonics should be axis=0.

Yes, because when using the numpy.linalg.lstsq(a, b) https://numpy.org/doc/stable/reference/generated/numpy.linalg.lstsq.html function I pass parametrer b, with the real and imag part for each hamonic there, that is the way I found to relate coefficients in the parametres a and b in lstsq, there maybe a better way. I did this because we need to apply lstsq for each (real, imag) pair in the image.

I will update the PR soon when I finish your review and corrections. And we can find a better way to solve the linear eqaution, do you know or suggets another alternative to numpy.linalg.lstsq(a, b)?

Thanks

I have updated the PR but I haven´t finished yet, so it has many unchecks test, I let you know when its ready for review and pass all checks

@cgohlke
Copy link
Member

cgohlke commented Sep 3, 2024

because we need to apply lstsq for each (real, imag) pair in the image.

I understand that the inputs for lstsq need to be in a certain way. But you already generate those in manual loops. Just change the indexing from [..., j] to [j, ...]. No?

All PhasorPy functions should ideally work with N-dimensional arrays. This function seems hard-coded for two dimensions in addition to harmonic. How about using numpy iterators instead? Or better vectorize the loop(s) if possible.

Also make sure the function works with NaN input. If numpy.linalg.lstsq does not work with NaNs, maybe scipy.linalg.lstsq is a better choice?

@schutyb schutyb closed this Sep 30, 2024
@schutyb schutyb reopened this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgohlke cgohlke cgohlke left review comments

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

At least 1 approving review is required to merge this pull request.

Assignees

@schutyb schutyb

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

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