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

[BUGFIX] Correct grid coordinates for ngrid=1 #1008

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Conversation

gogannes
Copy link
Contributor

[BUGFIX] Correct grid coordinates for ngrid=1

When ngrid=1 was chosen, np.linspace would just return the starting value of -1 * pt which would not place the point at the center of the rotor. With this fix the center of the rotor is chosen.

Related issue

#1004

Impacted areas of the software

The generation of the y and z coordinates for each of the turbines is affected in cases where ngrid=1 is chosen.

Additional supporting information

See #1004

Test results, if applicable

Locally all existing tests run without issues. The example case of #1004 now works as expected.

Copy link
Collaborator

@paulf81 paulf81 left a comment

Choose a reason for hiding this comment

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

The change seems very reasonable to me, and I was able to pull down the PR and run the tests successfully on my mac. I'm comfortable approving. Thank you @gogannes !

@rafmudaf
Copy link
Collaborator

rafmudaf commented Nov 8, 2024

The CI infrastructure has gotten quite stale since v2. To address the failing tests, I suggest to remove macOS from the test matrix and stick with only Linux. In any case, this is consistent with what we've done in v3 and v4. You can make that change here: https://github.com/gogannes/floris/blob/v2/.github/workflows/continuous-integration-workflow.yaml#L28.

@misi9170
Copy link
Collaborator

misi9170 commented Nov 8, 2024

The CI infrastructure has gotten quite stale since v2. To address the failing tests, I suggest to remove macOS from the test matrix and stick with only Linux. In any case, this is consistent with what we've done in v3 and v4. You can make that change here: https://github.com/gogannes/floris/blob/v2/.github/workflows/continuous-integration-workflow.yaml#L28.

Thanks @rafmudaf ---that sounds good. I'll create a separate PR that does that; and we can merge that first, then this one.

EDIT: The PR removing Mac OS tests is #1020

@misi9170 misi9170 added v2.x Related to any version of FLORIS less than v3 bug Something isn't working labels Nov 8, 2024
@misi9170 misi9170 merged commit c66a185 into NREL:v2 Nov 8, 2024
4 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v2.x Related to any version of FLORIS less than v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants