pyerrors icon indicating copy to clipboard operation
pyerrors copied to clipboard

[Fix] Remove mutables as default argument and fix smaller issues

Open fjosw opened this issue 1 year ago • 2 comments

In this PR I remove all mutables from default arguments (see for example this article).

I also corrected smaller stilistic issues and added missing docstrings.

fjosw avatar Dec 24 '24 16:12 fjosw

Hi! Thanks a lot for these changes - I was not aware of the issue... We would have to modify the call to covariance in the fit function. I think that we intended to have the possibility to pass **kwargs because it was possible to make choices regarding what method is used to compute the covariance matrix. In principle, a matrix that differs from the standard one, could now be passed directly to the routine. One could also think to explicitly check the **kwargs of the fit function for entries that are relevant for the covariance matrix. With the current implementation the parameter smooth cannot be passed on... What do you think?

s-kuberski avatar Jan 10 '25 10:01 s-kuberski

It should be sufficient to change https://github.com/fjosw/pyerrors/blob/4089238ddde38014a40f02b2e991f0c15b1a9dc0/pyerrors/fits.py#L369 to something like

corr = covariance(y_all, correlation=True, smooth=kwargs.get('smooth', None)) 

to get rid of the error and to still allow to use the smoothing here. If we don't pass on the keyword, this would be a breaking change that we could at some point implement, but maybe only with a deprecation warning where we check if the keyword has been passed.

s-kuberski avatar Feb 17 '25 14:02 s-kuberski