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] Add demo_cp_ct_surface location to data files. #1095

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

misi9170
Copy link
Collaborator

@misi9170 misi9170 commented Apr 3, 2025

I believe this should address #1094 . I'm not sure what the best way to test the fix is, though, short of merging this and creating a new release (UPDATE: Read comments below for how to test locally).

UPDATE:
To avoid this type of bug being introduced again, as well as adding the missing data to the [tool.setuptools.package-data] list of pyproject.toml, this PR:

  • Makes github workflow installations of floris non-editable by removing -e flag
  • Moves tests into a subdirectory to avoid finding the data (still don't totally understand why this is necessary, but it has the desired effect).

@misi9170 misi9170 added the bug Something isn't working label Apr 3, 2025
@rafmudaf
Copy link
Collaborator

rafmudaf commented Apr 3, 2025

You can test this locally by doing pip install floris/ without the typical -e in a new Python environment. Then, you can search for the file in the installation directory to verify it's there and also run the demo case.

@misi9170
Copy link
Collaborator Author

misi9170 commented Apr 3, 2025

@rafmudaf 's testing suggestion did work locally. I'm now testing removing the -e flag from github workflows to see if that would have identified this issue earlier (thanks, @rafmudaf , for the suggestion). Assuming the tests fail in the expected way, I'll re-add the package data. If test/examples then pass, this should be good to go.

@misi9170 misi9170 force-pushed the bugfix/missing-data branch from 1c01ae4 to db65d0f Compare April 4, 2025 14:44
@misi9170
Copy link
Collaborator Author

misi9170 commented Apr 4, 2025

Expected failures are now happening at db65d0f. Now re-including the data in pyproject.toml at 6af0b44.

@rafmudaf rafmudaf force-pushed the bugfix/missing-data branch from 0d522f3 to 67ab441 Compare April 4, 2025 19:02
@misi9170
Copy link
Collaborator Author

misi9170 commented Apr 4, 2025

Ok, after a lot of digging, @rafmudaf and I found and fixed a couple of other issues:

  • added wrg_test.wrg to tests/data to avoid relying on data in the examples directory
  • added -rL flag to copies to resolve symbolic links
  • add turbine_library/*.csv to [tool.setuptools.package-data] so that multimensional turbine data is available with Floris is installed using PyPI
  • created more robust paths using the package root for data loading in tests

@misi9170 misi9170 requested a review from rafmudaf April 4, 2025 20:30
@rafmudaf
Copy link
Collaborator

rafmudaf commented Apr 4, 2025

Thanks for all the digging here @misi9170. Also, I appreciate the status updates and documentation you've included in this PR. Nicely done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants