[MNT] Remove mutable objects from defaults

[eugenio-mercuriali]

Description

This PR is a fix for #1668. Removes mutable default arguments and replaces them with internal newly initialized mutable defaults.

Checklist

• Linked issues (if existing)
• Amended changelog for large changes (and added myself there as contributor)
• Added/modified tests
• Used pre-commit hooks when committing to ensure that code is compliant with hooks. Install hooks with pre-commit install.
  To run hooks independent of commit, execute pre-commit run --all-files

[fkiraly]

Nice!

I think this is great and can be in a single PR.

Some small issues:

• the tests are failing due to type annotations, have a look at the logs
• not a strict requirement, but it might be safer in case of __init__ to write the resolved argument to a parameter self._parametername, to ensure self.parametername is always identical with the __init__ input. This is a "dataclass-like" interface which may be useful later in interfacing sklearn and similar libraries.

[eugenio-mercuriali]

Nice!

I think this is great and can be in a single PR.

Some small issues:

• the tests are failing due to type annotations, have a look at the logs
• not a strict requirement, but it might be safer in case of __init__ to write the resolved argument to a parameter self._parametername, to ensure self.parametername is always identical with the __init__ input. This is a "dataclass-like" interface which may be useful later in interfacing sklearn and similar libraries.

I see, thanks for the feedback!
Added some changes - how does this look?

[fkiraly]

Looks great!

Though, for the __init__-s, you would still do self.paramname = paramname, just not change it, e.g.,

self.groups = groups
self._groups = do_sth_with_groups(groups)

so the class behaves like a dataclass with respect to its init parameters.

There were also a lot of linting failures related to the introduction of stricter requirements in ruff, I fixed most of them. The remaining ones seem genuine code issues, could you have ea look?

[codecov]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

[eugenio-mercuriali]

Looks great!

Though, for the __init__-s, you would still do self.paramname = paramname, just not change it, e.g.,

self.groups = groups
self._groups = do_sth_with_groups(groups)

so the class behaves like a dataclass with respect to its init parameters.

There were also a lot of linting failures related to the introduction of stricter requirements in ruff, I fixed most of them. The remaining ones seem genuine code issues, could you have ea look?

Just for me to understand, let's take the __init__ of encoders.py as an example:
if instead of:

self._groups = list(groups) if groups is not None else []

we do:

self.groups = groups
self._groups = list(groups) if groups is not None else []

then we would have to refactor usages of self.groups to self._groups in order to use the resolved argument within the class (+ no need to create a groups property) - is this what you suggest?

[fkiraly]

is this what you suggest?

Yes, exactly what I mean.
I confirm your understanding agrees with mine, @eugenio-mercuriali.

The high-level rationale is that the class then behaves, at all times of usage, like a dataclass with respect to its init parameters.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[eugenio-mercuriali]

is this what you suggest?

Yes, exactly what I mean. I confirm your understanding agrees with mine, @eugenio-mercuriali.

The high-level rationale is that the class then behaves, at all times of usage, like a dataclass with respect to its init parameters.

@fkiraly added changes based on the above. Could you take a look?

[eugenio-mercuriali]

@fkiraly are there any adjustments needed or good to merge?

[fkiraly]

Looks good to me!

Sorry, I thought I had replied.