search

OxIonics / ionics_fits

Small python fitting library with an emphasis on Atomic Molecular and Optical Physics
Apache License 2.0
0 stars 0 forks source link

Add derived parameter "contrast" to Sinusoid model #50

Closed AUTProgram closed 1 year ago

hartytp commented 1 year ago

Could you add a note to the docstring to remind people what we mean by contrast plz?

AUTProgram commented 1 year ago

Ah yes, will do

AUTProgram commented 1 year ago

Reading through the docstring for derived params, I think this line is a bit unclear:

- min/max: min / max values of the undamped sinusoid (including the offset and
          decay).

It says "undamped" sinusoid, so why does it mention "including the decay"? Since these are the global peak values that the undamped sinusoid takes, the offset doesn't matter either? Should we not just remove the whole part in parentheses?

AUTProgram commented 1 year ago

While I was at it, I also added functionality to find positions and values of maximum/minimum for the damped sinusoid (as suggested in the TODO section for derived params). Should I expand scope of this PR or submit a separate one?

AUTProgram commented 1 year ago

Another thing I was wondering about is whether we should set the lower bound for tau to zero by default. I think in principle negative taus (corresponding to exponentially increasing amplitude) are currently allowed if the user sets fixed_to = None. This is fine for the most general case of a sinusoid with an exponential factor, but in practice we would usually expect positive tau. This is also reinforced by the fact that the docstring mentions "decaying" or "damped" sinusoid in several places.

hartytp commented 1 year ago

It says "undamped" sinusoid, so why does it mention "including the decay"? Since these are the global peak values that the undamped sinusoid takes, the offset doesn't matter either? Should we not just remove the whole part in parentheses?

Yes, that's a sensible idea.

hartytp commented 1 year ago

While I was at it, I also added functionality to find positions and values of maximum/minimum for the damped sinusoid (as suggested in the TODO section for derived params). Should I expand scope of this PR or submit a separate one?

I'm happy either way.

hartytp commented 1 year ago

Another thing I was wondering about is whether we should set the lower bound for tau to zero by default. I think in principle negative taus (corresponding to exponentially increasing amplitude) are currently allowed if the user sets fixed_to = None. This is fine for the most general case of a sinusoid with an exponential factor, but in practice we would usually expect positive tau.

That's a sensible idea (defaults covering the most common case is good!).

This is also reinforced by the fact that the docstring mentions "decaying" or "damped" sinusoid in several places.

I wonder if we should change these words since the code is the same code one would have for an exponentially increasing sinusoid as well. This was me being a little careless.