Skip to content
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

Sigmoid.__call__ does not take lambda and gamma #170

Closed
pberkes opened this issue Nov 26, 2024 · 5 comments · Fixed by #184
Closed

Sigmoid.__call__ does not take lambda and gamma #170

pberkes opened this issue Nov 26, 2024 · 5 comments · Fixed by #184

Comments

@pberkes
Copy link
Collaborator

pberkes commented Nov 26, 2024

At the moment, Sigmoid.__call__ does not take the parameters lambda and gamma. It is odd, because the other public methods, Sigmoid.slope and Sigmoid.inverse to take those parameters into account.

The smell comes out in psignifit.tools.psychometric, which does take those parameters, and where the values are computed by calling Sigmoid, then rescaling:

    sigmoid = sigmoid_by_name(sigmoid_name)
    sigmoid_values = sigmoid(stimulus_level, threshold=threshold, width=width)
    psi = gamma + (1.0 - lambda_ - gamma) * sigmoid_values

I think this simple operation should be performed in Sigmoid.__call__

@pberkes
Copy link
Collaborator Author

pberkes commented Nov 26, 2024

Also worth noting that Sigmoid.__call__ does mention lambda and gamma in the docstring, but they are not used

@otizonaizit
Copy link
Collaborator

The other use of Sigmoid.__call__ that I could trace is here:

psi = sigmoid(level, thres, width) * scale + gamma

where scale is defined like this:
scale = 1 - gamma - lambd

so, yes, that seems to be a relic of the past and we should move lambda and gamma to the Sigmoid.__call__ method. It would be important to trace where we call Sigmoid objects though, as I wouldn't trust that we are testing all code paths where this may be the case. Not sure how to go about it.

And not that we are at it, maybe get rid of __call__ altogether and transform it into something more explicit like values() or so?
It probably looked like a good idea to use __call__ when the Sigmoid class was first developed, but I now think that saving a couple of characters when getting the values of the sigmoid is not worth the opacity of the intent of the call.

@otizonaizit
Copy link
Collaborator

by the way, this plan is exactly the opposite of what @pberkes proposed in #140 ;)

@pberkes
Copy link
Collaborator Author

pberkes commented Nov 27, 2024

Haha you got me. The point is that we need to do something about it.

In the end, it's related to #102: how is the user supposed to interact with a sigmoid, when they need to? it's a bit of a patchwork now

@pberkes
Copy link
Collaborator Author

pberkes commented Dec 15, 2024

Need to check psigniplot, as well:

            y = sigmoid(sigmoid_x, this_sigmoid_params['threshold'], this_sigmoid_params['width'])
            y = (1 - estimate['gamma'] - this_sigmoid_params['lambda']) * y + estimate['gamma']

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants