Remove default values used as expressions in generate.py.

[h-mayorquin]

This should only be missing durations as @JoeZiminski volunteer himself to do that. See:
#2342

[alejoe91]

Thansk @h-mayorquin

Can you check why tests are failing? It seems related to the change!

[h-mayorquin]

@alejoe91
The error was caused by the fact that np.array(0) behaves as False. I should know better to not play with the truthiness of containers.

However, as in this case, you actually ask the split_ids to be non-empty I Just made the argument a required one.

[samuelgarcia]

I am not sure to like this kind of changes.
I do not think we have the cases given by @JoeZiminski which is a inplace modification of the input variable.

[h-mayorquin]

@samuelgarcia
I think that the error is unlikely to show up here but setting expressions as default values is a well known footgun in python and a in consequence a practice. What's the gain of keeping a bad practice? You don't like how it looks?

[JoeZiminski]

Hey @h-mayorquin @samuelgarcia sorry for the delay on this! I will open a PR in the second half of next week. I think it is important to remove any mutable default variables due to the unintuitive behaviour of mutable defaults in python.

I had a quick look through and it is true that the function does not do any in-place modification of the default arguments, and would probably not cause an bug in this instance at the present time. Nonetheless I think it is important to remove the pattern here and anywhere else it is found in the codebase for the following reasons:

1. The most important, in future someone might change the code such that an in-place modification of the variable takes place. This will instantly cause horrible bugs if not caught in code review. So for future proof / maintainability reasons it makes sense to change it now.
2. It exemplifies an antipattern in the codebase which contributors may see and copy when inappropriate. It signals that this pattern is okay / safe when in fact it is only safe in very specific circumstances. Using the robust pattern turns this into a teachable moment (Q: 'why do you bother assigning None just to change it to dict? 'A: Oh, because of the crazy way python handles mutable default arguments', 'Q: Thanks I did not know about that, I am learning so much working on the SI codebase 😍!'.)
3. For code-consistency reasons, this pattern will have to be changed in some signatures (where the function makes an inplace modification to the default argument). So we end up with two patterns for mutable defaults across the codebase which is confusing.
4. I can't think of any tangible benefits of keeping mutable defaults but definitely happy to hear!

[alejoe91]

Let's go forward with this guys. I think that the majority of us agree that we should follow good practice ;) @h-mayorquin could you solve conflicts?

[h-mayorquin]

Let's try.

[h-mayorquin]

@alejoe91 OK, I fixed them Let's merge because I got conflicts twice already.

[samuelgarcia]

What ??
Did we reach concensus on this ?

Here it is little, I do not care so much but I really do not want to this to be the unique rule for all other the place/
The code would be looks very very ungly.

For instance here:

• https://github.com/SpikeInterface/spikeinterface/blob/main/src/spikeinterface/core/generate.py#L1952
• https://github.com/SpikeInterface/spikeinterface/blob/main/src/spikeinterface/generation/drifting_generator.py#L249

[JoeZiminski]

I agree it is less neat but in this I think it is worth the protection it affords, also the alternative pattern gives opportunity for some extra documentation and isolation of kwargs. For example currently generate_probe_kwargs here elements are currently undocumented but moving these to a function allows them to have their own docstring.

def get_default_generate_probe_kwargs():
    """ docs"""
    return dict(...)

and in the function itself the dict is replaced by None and if generate_probe_kwargs is None: generate_probe_kwargs = get_get_default_generate_probe_kwargs(). It is a typical pattern to see so should not confuse people too much.

I do agree that the existing code is neater than the solution but in this case would advocate the change nonetheless - this really is a very dangerous pattern. Mutable default arguments are a python antipattern and there are many sources warning (a small selection below) against their use:

• Never make a function argument a mutable default
• Common Gotchas
• Python Mutable Defaults Are The Source of All Evil

including this interesting case where the pattern caused failure of a company website launch and 1+ month of heavy bug-fixing!