Hi @cgohlke, I created this PR so we can start discussing the approach of the higher level function phasor_calibrate. I haven't done the corresponding tests yet, but wanted to share beforehand to see if you consider the approach correct.

Particularly, I have questions with the optional arguments passed to other functions, such as phasor_from_lifetime and phasor_center. I left the option to pass them as kwargs, but I don't know if this is the best approach since they won't be directly visible when using phasor_calibrate. They would have to go to the documentation of those functions and I don't think is optimal. Maybe add something in the docstring of phasor_calibrate regarding those optional kwargs? What do you think.

I also renamed the phase0 and modulation0 parameters to phase_shift and modulation_ratio as discussed before.

I think the split into higher level phasor_calibrate and lower level phasor_transform functions makes a lot of sense.

I have questions with the optional arguments passed to other functions, such as phasor_from_lifetime and phasor_center. I left the option to pass them as kwargs, but I don't know if this is the best approach

This is a recurring issue.
I think in this case of few parameters forwarded to functions within the same library, the use of kwargs should be avoided. Instead, explicitly list the parameters and repeat the documentation. That way the user does not have to look up documentation elsewhere and can use hints from the IDE. Also MyPy can verify the types.
In other cases, for example, when forwarding many possible parameters to matplotlib functions, it does not make sense to list and document those parameters again.

I have questions with the optional arguments passed to other functions, such as phasor_from_lifetime and phasor_center. I left the option to pass them as kwargs, but I don't know if this is the best approach

This is a recurring issue. I think in this case of few parameters forwarded to functions within the same library, the use of kwargs should be avoided. Instead, explicitly list the parameters and repeat the documentation. That way the user does not have to look up documentation elsewhere and can use hints from the IDE. Also MyPy can verify the types. In other cases, for example, when forwarding many possible parameters to matplotlib functions, it does not make sense to list and document those parameters again.

That makes a lot of sense! We can approach future implementations following that guidelines.

also renamed the phase0 and modulation0 parameters to phase_shift and modulation_ratio as discussed before.

I can't seem to find a link to the previous discussion right now. I don't particular like the phase0 and modulation0 names, and phase_shift and modulation_ratio look nicer. However, I find those confusing, since (1) phase and modulation are practically always phase shifts and modulations relative to something, and (2) the phase shift and relative modulation in this case are very specific to calibration. In formulas (I am aware of), the phase shift and relative modulation used for calibration are usually denoted as φ0 and M0, hence I chose phase0 and modulation0. In the past I used phase_zero and modulation_zero. Maybe we can discuss and settle this in Monday's meeting?

We mentioned it in #32, but I agree that it may be confusing using phase_shiftand modulation_ratio because they are also used in other context, altough they look nicer and also are easier to understand what they mean. phase_zeroand modulation_zero can be a good approach. I prefer them to phase0and modulation0.

Maybe if we add some math equations to the docstring it can be more visual what phase_zeroand modulation_zeroare and we can use those names.

I agree, lets discuss on monday.

@cgohlke I forgot to ask what do you think about the inputs of the function. Should it be the already calculated real and imag for the image to be calibrated and reference, as it as of now, or should it be the signals and handle the phasor calculation internally to avoid the user one extra step?

Should it be the already calculated real and imag for the image to be calibrated and reference, as it as of now, or should it be the signals

I think the function should accept phasor coordinates, which are the central "data structure" of this library. Some file formats (REF, R64, IFLI) do not contain signals but phasor or polar coordinates. At some point we will add high level io functions that return phasor coordinates.

That's a good Idea, having higher level io functions that call phasor_calibrate and maybe can have an option to already return the calibrated phasor coordinates if desired. I like that approach much better as this allows greater versatility in phasor_calibrate

higher level io functions that call phasor_calibrate and maybe can have an option to already return the calibrated phasor coordinates if desired.

Sorry, that's not really what I meant. Usually, signals of reference measurements are in separate files and that will require separate io and calibration steps. Some file formats like R64, IFLI, FLIF already contain referenced data or contain calibration information. Only in those cases the phasor coordinates will be returned calibrated. Otherwise they need to be calibrated, which I would keep separate, something like:

mean, real, imag, frequency = io.phasor_from_ptu(filename)
mean_ref, real_ref, imag_ref, _ = io.phasor_from_ptu(filename_ref)
real, imag = phasor_calibrate(real, imag, real_ref, imag_ref, frequency, lifetime, ...)

Re phasor_calibrate documentation: how about adding a note or an example that it is a convenience function for:

phasor_transform(
    real,
    imag,
    *polar_from_reference_phasor(
        *phasor_center(
            reference_real, 
            reference_imag, 
            skip_axes, 
            method,
        ),
        *phasor_from_lifetime(
            frequency, 
            lifetime, 
            fraction, 
            preexponential, 
            unit_conversion,
        ),
    ),
)

@cgohlke I added the tests and modified the introduction tutorial. I think this is ready for review, but there may be some aspects to improve.

This is almost ready. Can you pull in the recent changes from main?

@cgohlke I am getting some errors in testing, I believe is with the fetch datasets, but I am not sure.

I am getting some errors in testing, I believe is with the fetch datasets, but I am not sure.

You can follow the "Details" links next to the failed test runs and see exactly what failed. There's also an option to re-run failed checks.

All tests pass now.

I just noticed that the skip_axes parameter is missing from phasor_calibrate. Is that intentional?

It would be nice to have a tutorial using a real world dataset that can demonstrate this and other advanced features of the new phasor_calibrate function. I'm not aware of any suitable dataset though.

I missed the skip_axes parameter, you are right. I will fix this in a PR.

It would be great to have an interesting dataset that we can use for this. We could generate an suitable dataset here but we can only get .fbd files, which are not so good for a tutorial I think. But we can have something and look for files from another lab with access to other hardware.

I think a small, multi-harmonic FBD dataset might be OK for this. The skip_axes parameter could be used on the harmonics, no? Maybe I am missing something. Anyway, it is not urgent, just something to keep track of...