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?

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