Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wind direction resampling 2 #946

Merged
merged 4 commits into from
Jul 18, 2024
Merged

Conversation

misi9170
Copy link
Collaborator

@misi9170 misi9170 commented Jul 15, 2024

I recently approved and merged #943, but then noticed after the merger that, when I switched double uses of np.append() to np.concatenate(), I hadn't propagated the change throughout the file.

This small PR addresses that issue and makes the use of np.concatenate() consistent throughout. It's not a bugfix, as the code runs fine, but is a very minor enhancement.

@paulf81 , I have left comment that it would be good to get your opinion on, on a small inconsistency between the upsample() methods of WindRose and WindTIRose (again, something I should probably have caught when reviewing #943):

In WindTIRose.upsample(), there is a code block:

        # If wd_range_min_current is less than 0, then pad the wind_direction column with
        # that value + 360 and expand the matrices accordingly (this avoids interpolation errors)
        if wd_range_min_current < 0:
            # Pad wind direction column with min_wd + 360
            wind_direction_column = np.append(
                wind_direction_column, np.min(self.wind_directions) + 360.0
            )

            # Pad the remaining with the appropriate value
            freq_matrix = np.concatenate((freq_matrix, freq_matrix[0, :, :][None, :, :]), axis=0)
            if self.value_table is not None:
                value_matrix = np.concatenate(
                    (value_matrix, value_matrix[0, :, :][None, :, :]), axis=0
                )

but this code does not exist in WindRose.upsample(). Is that expected, and to do with the difference between the two classes? If not, is it redundant in WindTIRose.upsample() or missing in WindRose.upsample()?

@misi9170 misi9170 added the enhancement An improvement of an existing feature label Jul 15, 2024
@paulf81
Copy link
Collaborator

paulf81 commented Jul 18, 2024

This is redundant in the WindTIRose case, it is now implemented within this block (starting at 1585):

  # In the alternative case, where the wind directions cover the full range
        # ie, 0, 10, 20 30, ...350, then need to place 0 at 360 and 350 at -10
        # to cover all interpolations
        else:
            # Pad wind direction column with min_wd + 360
            wind_direction_column = np.concatenate(
                (
                    [np.max(self.wind_directions) - 360.0],
                    wind_direction_column,
                    [np.min(self.wind_directions) + 360.0],
                )
            )

            # Pad the remaining with the appropriate value
            freq_matrix = np.vstack(
                (freq_matrix[-1, :, :][None, :, :], freq_matrix, freq_matrix[0, :, :][None, :, :])
            )
            if self.value_table is not None:
                value_matrix = np.vstack(
                    (
                        value_matrix[-1, :, :][None, :, :],
                        value_matrix,
                        value_matrix[0, :, :][None, :, :],
                    )
                )

Nice catch and sorry for the mistake ( I think mostly harmless, it's basically adding endpoints twice to the interpolation matrix) but the tests will confirm, I will delete and retest now

@misi9170 misi9170 merged commit d2c27ca into NREL:develop Jul 18, 2024
8 checks passed
@misi9170 misi9170 deleted the bugfix/wind-data-concat branch July 18, 2024 04:07
@misi9170 misi9170 mentioned this pull request Jul 18, 2024
4 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