Remove obsolete configuration options

[otizonaizit]

Get rid of configuration options that are not used in the code.

This PR also removes the configuration option stimulus_range. For a rationale, see #143
The stimulus range is now only inferred from the data. Note that width_min, i.e. the lower bound of the width prior, can still be manually set. I am not sure if we should get rid of this too...

Fixes #145

[pberkes]

Thank you @otizonaizit , the changes look good. If we get rid of stimulus_range, I think some of the notebooks in demo need to be modified.

Are we sure we want to remove it? In #143, the problem was that the minimum width was set in a naive way, and we have improved that. Felix' comment was that the experiment should normally cover the range of plausible stimuli, so setting it by hand is not useful. OTOH, there might be cases where it's not possible to cover the full range, and still want to infer a sigmoid on the full range?

[otizonaizit]

I'm not sure about stimulus_range either. I could split the PR, so that this one only covers the obvious things and move the stimulus_range related things on a different PR where we can have a proper discussion? I personally don't like complicating the API and the code to cover theoretically interesting use cases that haven't shown up in the practical experience of anyone who has ever used the code. But you assess if this is the case here I'd like to hear the psychophysicists in town ;) 27 Nov 2024 18:28:42 Pietro Berkes ***@***.***>:

…

Thank you @otizonaizit[https://github.com/otizonaizit] , the changes look good. If we get rid of *stimulus_range*, I think some of the notebooks in *demo* need to be modified. Are we sure we want to remove it? In #143[#143], the problem was that the minimum width was set in a naive way, and we have improved that. Felix' comment was that the experiment should normally cover the range of plausible stimuli, so setting it by hand is not useful. OTOH, there might be cases where it's not possible to cover the full range, and still want to infer a sigmoid on the full range? — Reply to this email directly, view it on GitHub[#171 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AACUYC67JF7EK5ZF3XANX4L2CX6MPAVCNFSM6AAAAABSTIIUGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBUGQZDIMZTHA]. You are receiving this because you were mentioned. [Tracking image][https://github.com/notifications/beacon/AACUYC4EK3ORPUX2ZZYJZNL2CX6MPA5CNFSM6AAAAABSTIIUGSWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUVIZ5ZE.gif]

[otizonaizit]

btw, talking with a heavy user of psychometric function fitting libraries, I heard that it is difficult to imagine what sense an experiment would make where you do no sample the lower or higher part of the stimulus range but then want to fit a psychometric function and use that fit to claim something about thresholds and such...
If it is only about synthetic data, then we can keep the option internally as private but there's not need to advertise it as something useful. My fear is that people will follow the example codes where it is used and do the same mistake we were doing in #143 ...

[pberkes]

alright, let's do it then

[pberkes]

There's one last thing to do: remove options["stimulus_range"] = stim_range from the notebook psignifit/demos/parameter_recovery_demo.ipynb. It appears in two places. In principle, the rest should continue working as intended.

[otizonaizit]

@guillermoaguilar : before proceeding with the removal of stimulus_range I'd like to get your OK too.

[guillermoaguilar]

Yes sorry for the silence.
stim_range is not really needed from the user perspective IMO.
It should be private.

There's one last thing to do: remove options["stimulus_range"] = stim_range from the notebook psignifit/demos/parameter_recovery_demo.ipynb. It appears in two places. In principle, the rest should continue working as intended.

In the rearrangement of documentation I've already removed it. But I haven't merge that yet. Leave that to me.

[otizonaizit]

There's one last thing to do: remove options["stimulus_range"] = stim_range from the notebook psignifit/demos/parameter_recovery_demo.ipynb. It appears in two places. In principle, the rest should continue working as intended.

well its' not so easy: that demo was already broken by #161 :

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[7], line 1
----> 1 assert np.isclose(res.parameter_estimate['lambda'], lambda_, atol=1e-4)

AttributeError: 'Result' object has no attribute 'parameter_estimate'

I think that the demos need to be checked again. I don't think it is a good idea for me to fix it in this PR, given that the thing is already broken...
By the way, it may make sense to still have a Result.parameter_estimate which is an alias of Result.parameter_estimate_MAP?

[guillermoaguilar]

I think that the demos need to be checked again. I don't think it is a good idea for me to fix it in this PR, given that the thing is already broken... By the way, it may make sense to still have a Result.parameter_estimate which is an alias of Result.parameter_estimate_MAP?

Yes, it should be an alias for _MAP.
It's too complicated to ask the user to decide if to use parameter_estimate_MAP or something else.... parameter_estimate should be the default.

[guillermoaguilar]

Then I guess I should merge the documentation changes and see what's broken.

[guillermoaguilar]

Documentation is merged, and yes it's broken in all the examples that use parameter_estimate, for example the basic usage example here
https://psignifit.readthedocs.io/en/latest/basic-usage.html#getting-results-from-the-fit

As I wrote I think it has to be an alias, it doesn't make sense to change the docs, it makes it more obscure and complicated for the user.

[otizonaizit]

OK, I made stimulus_range private. Which means that it can still be used internally with the name Configuration._stimulus_range but it is not advertised anywhere.