himoto / hillfit

Fitting the Hill Equation to Experimental Data
MIT License
11 stars 4 forks source link

Add `bottom_param` option #17

Closed freiburgermsu closed 2 years ago

freiburgermsu commented 2 years ago

Hello @himoto !

Here is my idea for the bottom_param option. It is a simple boolean argument, where False provides zero for the bottom parameter, as you described.

A raise catch is also included, which clarifies a potential error that the user might experience (I experienced it) depending upon the applied dataset.

freiburgermsu commented 2 years ago

Hello @himoto !

Yes, your edit is correct. Thank you for reviewing it :)

Andrew

himoto commented 2 years ago

@freiburgermsu, I have fixed the code above but we still need to resolve an IndexError

FAILED test/test_fitting.py::test_fitting - IndexError: tuple index out of range

freiburgermsu commented 2 years ago

Hello @himoto!

I resolved the bug in my latest push. The curve_fit function was essentially interpreting the bottom_param argument as being one of the parameters that must be calibrated. The location of redefining the bottom parameter was relocated downstream of this process to not interfere with it.

Thank you :) Andrew

himoto commented 2 years ago

Hi @freiburgermsu, thanks for resolving the issue. If I understand your code correctly, what you are trying to do is to assign 0 to bottom parameter after parameter fitting.

# here
if not bottom_param:
    popt[1] = 0

This does not look nice to me because there might be more appropriate parameter values for top, nH, or ec50 when bottom is fixed to 0.

In my opinion, the bottom parameter must be fixed to 0 during regression when bottom_param=True. Below is my idea. Currently, param[1] is assigned to bottom even when bottom_param=True.

def _equation(self, x: np.ndarray, *params) -> np.ndarray:
    self.top = params[0]
    self.bottom = params[1]
    self.ec50 = params[2]
    self.nH = params[3]

    return self.bottom + (self.top - self.bottom) * x ** self.nH / (
        self.ec50 ** self.nH + x ** self.nH
    )

I think this should be the following:

def _equation(self, x: np.ndarray, bottom_param: bool, *params) -> np.ndarray:
    self.top = params[0]
    self.bottom = params[1] if bottom_param else 0
    self.ec50 = params[2]
    self.nH = params[3]

    return self.bottom + (self.top - self.bottom) * x ** self.nH / (
        self.ec50 ** self.nH + x ** self.nH
    )

Then, the estimated parameter values are based on the assumption that bottom=0. I would be happy if you could give me comments on this plan.

Many thanks, Hiroaki

freiburgermsu commented 2 years ago

@himoto This may cause the same IndexError as before, since the error was originally caused by similar logic

self.bottom = 0
if bottom_param:
   self.bottom = params[1]

to what you propose. Essentially, the additional bottom_param argument in the _equation() function is incorrectly interpreted by Python to be the first entry of the params arguments, and thus an IndexError is launched at self.nH = params[3], since len(params)=3 at this point instead of 4.

The solution may be to simply define a second _equation_bottom() function for where the user desires the bottom parameter, and then remove the bottom parameter from the _equation() function where the user does not want the bottom parameter. The bottom_param argument would be passed through the _get_params() function, and then the proper equation function would be executed to acquire the proper set of parameters.

himoto commented 2 years ago

@freiburgermsu, thanks for the detailed explanation, now I understand the exact cause. The arguments of _equation(...) must be (self, x: np.ndarray, *params) and we must not add bottom_param here.

The solution may be to simply define a second _equation_bottom() function for where the user desires the bottom parameter, and then remove the bottom parameter from the _equation() function where the user does not want the bottom parameter.

Yes, this can resolve the issue, but what about moving bottom_param argument to __init__() (and setting default to True)?

def __init__(
        self,
        x_data: Union[List[float], np.ndarray],
        y_data: Union[List[float], np.ndarray],
        *,
        bottom_param: bool = True,
) -> None:
        self.x_data = np.array(x_data)
        self.y_data = np.array(y_data)
        self.bottom_param = bottom_param

Then, we can call bottom_param via self and do not need to create another equation function:

def _equation(self, x: np.ndarray, *params) -> np.ndarray:
    self.top = params[0]
    self.bottom = params[1] if self.bottom_param else 0
    self.ec50 = params[2]
    self.nH = params[3]

    return self.bottom + (self.top - self.bottom) * x ** self.nH / (
        self.ec50 ** self.nH + x ** self.nH
    )
freiburgermsu commented 2 years ago

@himoto Yes! This is a brilliant work-around.

himoto commented 2 years ago

@freiburgermsu thank you for your comments and updates!