Skip to content

Conversation

@paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Oct 21, 2024

Add tests of nans in inputs to wind data objects

Working with flasc, it's possible to generate data series where wind_directions or wind_speeds contain NaN values, in flasc this typically signifies missing or faulty data. However within the WindData objects in FLORIS, the presense of NaNs in certain inputs can yield ambiguous errors. Further, since inputs like wind_directions and wind_speeds within WindData objects will be used to drive FlorisModel simulations, containing NaN values will further generate additional errors at that point.

This pull request adds a check at inputs to of WindData objects that input arrays wind_directions, wind_speeds, turbulence_intensities and values do not contain NaN values. Some additional tests are added to confirm correct behavior.

@paulf81 paulf81 added the enhancement An improvement of an existing feature label Oct 21, 2024
@paulf81 paulf81 requested a review from misi9170 October 21, 2024 20:36
@paulf81 paulf81 self-assigned this Oct 21, 2024
@misi9170
Copy link
Collaborator

misi9170 commented Mar 3, 2025

Small test script:

import numpy as np
from floris import WindRose

wind_directions = np.array([0.0, 90.0, 180.0, 270.0])
wind_speeds = np.array([5.0, 10.0, 15.0])

print("Instantiating without NaNs")
WindRose(wind_directions, wind_speeds, ti_table=0.06)

# Introduce NaN values
wind_directions[1] = np.nan
print("Instantiating with NaNs")
WindRose(wind_directions, wind_speeds, ti_table=0.06)

On the develop branch, the following printout is generated

Instantiating without NaNs
Instantiating with NaNs
Traceback (most recent call last):
  File "/Users/msinner/projects/floris/examples/untracked/pr1007.py", line 13, in <module>
    WindRose(wind_directions, wind_speeds, ti_table=0.06)
    ~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/msinner/projects/floris/floris/wind_data.py", line 182, in __init__
    raise ValueError("wind_directions must be monotonically increasing")
ValueError: wind_directions must be monotonically increasing

With the change, this is updated to

Instantiating without NaNs
Instantiating with NaNs
Traceback (most recent call last):
  File "/Users/msinner/projects/floris/examples/untracked/pr1007.py", line 14, in <module>
    WindRose(wind_directions, wind_speeds, ti_table=0.06)
    ~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/msinner/projects/floris/floris/wind_data.py", line 179, in __init__
    raise ValueError("wind_directions contains NaN values")
ValueError: wind_directions contains NaN values

@misi9170
Copy link
Collaborator

misi9170 commented Mar 3, 2025

Playing around with this a bit more, I notice that passing wind condition inputs to FlorisModel.set() does not raise an error, and moreover, run() can still be successfully called.

For example,

import numpy as np
from floris import WindRose, FlorisModel

wind_directions = np.array([0.0, 90.0, 180.0, 270.0])
wind_speeds = np.array([8.0, 8.0, 8.0, 8.0])

# Introduce NaN values
wind_directions[1] = np.nan
print("Setting FlorisModel with NaNs")
fmodel = FlorisModel(configuration="defaults")
fmodel.set(layout_x=[0,500], layout_y=[0,0])
fmodel.set(
    wind_directions=wind_directions,
    wind_speeds=wind_speeds,
    turbulence_intensities=0.06*np.ones_like(wind_directions)
)
print(fmodel.core.flow_field.wind_directions)
fmodel.run()
print("Powers:", fmodel.get_farm_power())
print("AEP:", fmodel.get_farm_AEP())
print("AEP from powers:", np.nansum(fmodel.get_farm_power())*365*24 / fmodel.n_findex)

produces

Setting FlorisModel with NaNs
[  0.  nan 180. 270.]
Powers: [3507908.91835834              nan 3507908.91835834 2108945.22330688]
floris.floris_model.FlorisModel WARNING Computing AEP with uniform frequencies. Results results may not reflect annual operation.
AEP: 19983231101.45161
AEP from powers: 19983231101.45161

In particular, note that

  1. The second power from get_farm_power is nan. This seems to be reasonable behavior.
  2. The AEP is still calculated and returns a finite value. Perhaps interestingly, as shown in the last printout, this value assumes that when averaging the power over the conditions, there is 1/4 weight given to 0 power (i.e., the nan value is ignored in the numerator of the mean but not in the denominator). However, this is a special case where freq is generated automatically and uniformly; in reality, a provided freq value would presumably not contain a nonzero value for a wind direction corresponding to nan.
  3. If there is only one turbine (e.g. fmodel.set(layout_x=[0], layout_y=[0])) and one of the wind_directions is nan, none of the output powers are nan. This is ok; the wind direction does not need to be known to compute the power if there is only one turbine. However, as expected, if one of the wind_speeds is nan, the corresponding farm power is nan. So, nothing too concerning there, but perhaps an interesting outcome.

All of this is to respond to a previous discussion about whether nan values should be handled within the Floris core code. I think they are handled reasonably---an error is not raised if a nan wind condition is provided, but output powers for that condition are returned as nans. There is some ambiguity about what this means for the AEP output, but for now, I do not think this is a major issue.

np.testing.assert_allclose(wind_ti_rose.turbulence_intensities, expected_result)


def test_wind_rose_nan_values():
Copy link
Collaborator

@misi9170 misi9170 Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests don't seem to actually test the desired behavior. They pass on both the develop branch (without the new NaN check) and with the new code, because in both cases, a ValueError is raised.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now fixed this by adding a match kwarg to pytest.raises()

@misi9170
Copy link
Collaborator

misi9170 commented Mar 3, 2025

@paulf81 I've cleaned this up a bit. However, there are some tests now failing because there is not a NaN check in the TimeSeries.__init__(). If you think there should be, do you mind adding that in? Alternatively, following my earlier comment about FlorisModel.set() allowing NaNs, perhaps TimeSeries should allow NaN values---in this case, the test can be removed.

Note that the test for TimeSeries was originally passing erroneously, because a ValueError was indeed raised but because the lengths of wind_directions and wind_speeds did not match, rather than because they contained NaNs.

@misi9170
Copy link
Collaborator

Conversation on this topic moved to #1076, and (although I let it drop for a while), hopefully we are close to a resolution there.

@misi9170 misi9170 mentioned this pull request Dec 24, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement An improvement of an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants