-
Notifications
You must be signed in to change notification settings - Fork 6
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
base: main
Are you sure you want to change the base?
Conversation
calculate phasor coordinates from frequency-domain signals convert between phasor coordinates phase and modulation apparent single lifetimes lifetime mixtures with fractional intensitie
…92/phasorpy into lifetime-module-branch
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 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
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. |
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. |
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. |
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. |
No problem, best of luck to @bruno-pannunzio ! |
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:
Considering the code you proposed in your PR, I believe that there are some functions that can be partly covered by your code:
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. |
Hi @Mariochem92, the PR #28 has been merged to main and the following functions listed before were already implemented:
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. |
Hi @Mariochem92, I think the following functionality from this PR has been implemented:
|
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.