Fix issue with mutable default attributes

[paulf81]

Fix issue with mutable default attributes

This pull request address the fact that if you use a list, dict, array or other mutable object as a default parameter in attrs (or more generally in function defaults), all instances of the declared object will point to the same list/dict/array, possibly generating confusing bugs. See Issue #758 for more detail on the problem and proposed solution.

The approach used in this pull request was determined in Issue #758 and is also identified as the correct solution in the attrs documentation (https://www.attrs.org/en/stable/api-attr.html#attr.ib) (can see also a quick explanation here: https://refex.readthedocs.io/en/latest/guide/fixers/attrib_default.html), specifically to use a factory to generate a new list/dict/array, rather declaring a single instance. The resulting changes all take the form of converting:

power: NDArrayFloat = field(default=[], converter=floris_array_converter)

to:

power: NDArrayFloat = field(factory=list, converter=floris_array_converter)

For empty lists, while using a lambda for empty numpy arrays:

w_initial_sorted: NDArrayFloat = field(init=False, factory=lambda: np.array([]))

Related issue

Closes #758

Impacted areas of the software

farm.py
turbine.py
flow_field.py
wake.py
empirical_gauss.py

Test results, if applicable

[RHammond2]

I thinks is great! My only requested change is to pick one approach for the empty object creation process, like I mentioned in my comment on turbine.py. I'm not really picky (even if I've demonstrated a preference in the commecnts) about which it ends up being, so long as it's consistent (which is really focused on future maintenance).

[RHammond2]

For instances like this below, I would recommend modifying slightly to ensure the typing is valid (even if unchecked currently) to use the following paragdigm:

x: NDArrayFloat = field(init=False, factory=list, converter=floris_array_converter)

Essentially what will happen if typing is implemented, without converting to the array type, it will fail. Additionally it creates some confusion (though some already exists tbc) to create a list for an object indicated to be an array.

The only place this will cause issues is with non-float arrays, in which case I'd recommend having two variations of floris_array_converter such as float_array_converter and object_array_converter. I don't know that it needs to have floris_ at the front at this point, and might be a lingering (but dated) idea from early in the design process.

Because this is a suggested course correction somewhat late in the game, i could see this being a separate issue/PR since that usage is pretty common.

[RHammond2]

My comment from farm.py stands for this as well since that would have the same effect without a lambda for an empty object, but is also not a required change.

[RHammond2]

I guess I'd say we should have a unified approach now that I see it's a separate approach for the same results in each of the files so far (love Python for this!), at least to make it easier for future developers.

[paulf81]

Hi @RHammond2 , thank you for your review! I opened a new issue (#761) to capture your request, would you mind to both check if that issue captures the problem/solution well? And then also would you mind to re-review this pull request and let us know if we can merge? Thank you!

[RHammond2]

@paulf81, I think I see where this is going, which is to simply translate to a version that isn't causing memory issues under certain use cases, and then to decide on the FLORIS standard later. In this case, the code is approved, and a decision can be made later about the best approach for FLORIS.

[paulf81]

Thank you @RHammond2 !