Speed up turbine loading operation

[misi9170]

@Bartdoekemeijer points out in #965 that instantiating the FlorisModel can be slow, especially when there are many turbines. @achenry had also pointed out a similar problem in #885.

After looking into this some, and using the profiling method suggested by @achenry, I found that a lot of time is being spent in the construct_turbine_map() method of Farm. Essentially, each Turbine is being instantiated separately, even if all turbines are the same type (which is often the case).

This PR proposes a fix to this that only calls Turbine.from_dict() once for each type of turbine, and then uses those (fewer) instantiations to construct the Farm.turbine_map. I'm seeing a substantial speed up, and now I think (after the change) that instantiation speed is dominated by n_findex rather than n_turbines.

There may be other opportunities for speed up during instantiation, but I think this may be the lowest hanging fruit.

Notes

I took the opportunity to also partly address #864, which raises the issue that the "turbine_type" key of the turbine dictionary must be changed in order to implement changes to other parts of the dictionary. While the changes I have made here do not necessarily "fix" the issue, now an informative error is raised when this situation arises.

Affected areas of the code

• floris/core/farm.py

Tests

• All existing tests pass
• New test for checking that an error is raised if two different turbine definitions are given with the same "turbine_type"
• New test that checks that modified turbine definitions make it into the turbine_map correctly

Examples

The main relevant example is examples/examples_turbine/002_multiple_turbine_types.py. I see no change to the outputs, as expected.

Performance

Using the example code in #965, I get the following performance on the develop branch:

while this PR provides:

[paulf81]

Here is also the wrote from running it on python 3.11 on my machine:

[paulf81]

Reviewing the code and checking the tests this all looks good. the only thought I had, would 1 or 2 additional tests where the number of unique turbine types is different than the number of turbines (and some assertions about what should be observed) be good to add while we're at it? Or is that all sufficiently covered by existing tests?

[misi9170]

Looking across the speed results here and on #965, it seems that the "remaining" time between the n_findex=300 and n_findex=3000 cases is approximately 5 seconds across various machines (or more like 3 seconds when using python 3.11) when setting 1000 turbines.

[misi9170]

Reviewing the code and checking the tests this all looks good. the only thought I had, would 1 or 2 additional tests where the number of unique turbine types is different than the number of turbines (and some assertions about what should be observed) be good to add while we're at it? Or is that all sufficiently covered by existing tests?

Yeah, I could add a quick test for that. The test I've added currently have 5 turbines, one of which is different, but I should check that now if "turbine_type" is renamed, the setup goes correctly.

[misi9170]

Slight update on the timings where I'm no longer rounding down the time to the nearest integer (Running on Mac with python 3.10.14):

[misi9170]

Reviewing the code and checking the tests this all looks good. the only thought I had, would 1 or 2 additional tests where the number of unique turbine types is different than the number of turbines (and some assertions about what should be observed) be good to add while we're at it? Or is that all sufficiently covered by existing tests?

I have now added this test

[paulf81]

Good to go from me then!

[Bartdoekemeijer]

Awesome!

Here's my performance with 3.11.8 on WSL with Ubuntu 22 LTS:

Thank you so much for fixing this, and especially on such short notice!!

[paulf81]

Awesome!

Here's my performance with 3.11.8 on WSL with Ubuntu 22 LTS:

Thank you so much for fixing this, and especially on such short notice!!

A nice improvement!

[bayc]

@misi9170 Yea, this looks nice! Just one suggestion in my comment above.

[misi9170]

@misi9170 Yea, this looks nice! Just one suggestion in my comment above.

This is now done