Hi @cgohlke, thanks for improving the docstrings of the functions that had some errors. I don't think this overlaps with anything I am working on so no problem with that. I like the idea very much of starting to implement some thing in cython to speed up the processing, maybe we can talk about it today in the meeting.

I also believe that the tests should be improved as there are many cases that are not testes, we can focus on improving this on the following weeks.

Regarding the phasor_from_signal implementation, I believe Bruno has been researching on ways to improve it and also to accept different fft functions, so maybe we can also discuss today this aspect.

Hi @bruno-pannunzio . Thanks for the feedback.

OK, I will remove phasor_from_signal from this PR (assuming #20 is imminent) but leave phasor_from_signal_f1.

Besides the tests marked with TODO:, any other test cases need to be improved in this PR?

I think those are ok. I think they also apply to the tests from my previous PR because not all functions cover all those test cases you added to the TODO. So I will improve those tests also when possible.

Regarding today's discussion about using plain SI units: should I integrate it into this PR as a commit? Then we'll see how things look and if we don't like can just revert the commit...

That would be great. If you need I can also do some work on it but may be caotic if we both do stuff at the same time. Let me know If you want me to work on something related.

Also, before I forget although is not related to this PR. I was reading some papers and some terms used for transforming the coordinates are 'phase shift' and 'modulation ratio'. Could it be a good idea to rename phi0 and mod0 to phase_shift and mod_ratio or maybe leave it that way? I think it sounds better but I don't know if it is more easy to understand.

Yes, I like phase_shift and modulation_ratio as input for phasor_calibrate and returns for polar_from_reference. It still needs to be communicated that it's regarding the reference/calibration? For acquired data, phase and modulation are always phase shifts and modulation ratios of some kind...

You are right, maybe its something that we can discuss if it make sense or if it can lead to misunderstandings. I will get feedback on this topic.

Regarding today's discussion about using plain SI units: should I integrate it into this PR as a commit? Then we'll see how things look and if we don't like can just revert the commit...

OK, that did not go too well. I now think we should not handle/store lifetimes and frequencies in s and Hz. The precision of 32-bit floating point float32 is not enough to accurately represent lifetimes in ns. It is enough for frequencies in MHz, but not GHz. 64-bit floating point (double) is good enough to store these values in s and Hz, but precision still might become an issue when moving to the ps/THz range...

Anyway, I think the current implementation of phasor_from_lifetime does not strictly require users to pass frequencies and lifetimes in MHz and ns units. They might as well pass them as, for example, THz and ps or Hz and ms (phosphorescence). The function could also accept an optional conversion factor (1e-3 by default) as a function parameter, so users don't have to covert their lifetime or frequency values before passing them. Similar for the phasor_to_apparent_lifetime function...

Interesting benchmark results: the phasor_from_signal_f1 function is 10 to 100 times faster than the phasor_from_signal reference numpy implementation on a 256x256x256 uint8 stack (lifetime_cat.tif). Maybe this computation is extremely memory limited or numpy converts the stack to double precision?

timeit.timeit('phasor_from_signal(stack, axis=0)', number=10, globals=globals())
6.033
timeit.timeit('phasor_from_signal_f1(stack, axis=0)', number=10, globals=globals())
0.524
timeit.timeit('phasor_from_signal_f1(stack, axis=0, numthreads=28)', number=10, globals=globals())
0.063

Hi @bruno-pannunzio. I think this is ready for review.

You can download the docs, including formulas and tutorial as a build artifact at https://github.com/phasorpy/phasorpy/actions/runs/7825054348?pr=32

Never mind the long phasor_plot function in the tutorial. The plan is to move it to a phasorpy.plot module.

I see the following issues with the phasor_from_signal_f1 function:

1. A copy of the input signal array is made if it is not C contiguous. That is necessary because the array is reshaped to a 3D memoryview. This could be avoided by using the numpy iterator C API, but the implementation would be less straightforward/readable/maintainable and not compatible with OpenMP (I think).
2. On macOS, Xcode does not include OpenMP libraries. There's a workaround using clang, but that could break anytime and might conflict with workarounds used in other Python libraries.
3. Related to that, a slower than necessary code path is used when setting num_threads > 1 on builds without OpenMP.
4. Only the phasor coordinates of the fundamental frequency is calculated.

Hi Christoph, sorry I didn't reply before, I was on a trip until yesterday. I will go through your comments and will get back to you on the aspects you pointed out.

Hi @cgohlke, I tested the phasor_from_f1 and phasor_from_lifetime functions and both work great. I tried all the pipeline for calibration with real images (fbd and ptu) and it works as expected using both. Also I timed the phasor calculation from signal of ptu files which are quite large. Your implementation with cython is much faster than using numpy fft. Using phasor_from_signal_f1 took just 2.8 seconds, while the numpy fft implementation took 13.5 seconds, so this is a great improvement.

I also tested the num_threads parameters but had not much of an impact with different numbers tested. Maybe as I am running with macOS, this is not compatible as you stated before.

The tutorial looks really good and easy to understand. The only thing I see is a bit distracting is the code for phasor_plot at the beginning, but as you said before, when that is implemented in phasorpy.plot, I'm guessing it will disappear from the tutorial and it will be cleaner.

Thanks for the feedback.

I also tested the num_threads parameters but had not much of an impact with different numbers tested. Maybe as I am running with macOS, this is not compatible as you stated before.

Yes, unfortunately there's no official support for OpenMP on macOS/Xcode. It's unbelievable. I applied the workaround in the lfdfiles wheels, which ship with their own copy of OpenMP. I 'm not sure it's the right solution for this project.

The only thing I see is a bit distracting is the code for phasor_plot at the beginning, but as you said before, when that is implemented in phasorpy.plot, I'm guessing it will disappear from the tutorial and it will be cleaner.

Exactly, that function will be moved to phasorpy.plot. I have done some work on that. It will be in another PR.

Hi @bruno-pannunzio Do you think this can be merged?

@cgohlke I was just going to comment that I believe this can be merged and we can keep working/discussing the points were we can have more input.

Btw, wheels for the main branch are available at https://github.com/phasorpy/phasorpy/actions/workflows/build_wheels.yml. Just follow the latest workflow run and go to artifacts, for example, https://github.com/phasorpy/phasorpy/actions/runs/7933102707#artifacts