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

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

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

fjosw
Copy link
Owner

@fjosw fjosw commented Dec 24, 2024

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 fjosw marked this pull request as draft December 24, 2024 16:55
@s-kuberski
Copy link
Collaborator

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
Copy link
Collaborator

It should be sufficient to change

corr = covariance(y_all, correlation=True, **kwargs)

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.

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

Successfully merging this pull request may close these issues.

2 participants