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

Move utils package under windIO #18

Merged
merged 2 commits into from
Oct 24, 2023
Merged

Conversation

rafmudaf
Copy link
Collaborator

@rafmudaf rafmudaf commented Sep 3, 2023

This pull request changes the directory structure of the repository so that the utils package is included under windIO rather than beside it. I have not discussed these changes with windIO developers, so please feel free to discuss here or disregard this pull request if it isn't inline with the intended design of windIO.

Directory reorg

Currently, pip-installing windIO results in two (actually three, see below) packages being installed: windIO and utils. This leads to import statements like the following from the plant-tests:

from utils.yml_utils import load_yaml
from utils.pywake_utils import ymlSystem2PyWake
from utils.pywake_utils import yml2Site, xr2Site

Within the windIO repository, the context is clear. However, when used outside of this repository, utils is vague and leads to ambiguity.

This pull request moves the utils directory under the windIO directory so that only one package is installed. The import statements then read like this:

from windIO.utils.yml_utils import load_yaml
from windIO.utils.pywake_utils import ymlSystem2PyWake
from windIO.utils.pywake_utils import yml2Site, xr2Site

For context, I'm using windIO in another project, and this reorganization of the modules improves the readability of the new software.

Installation config

Additionally, the test directory is currently installed since the find_package(exclude configuration in setup.py does not include a wildcard (*). This pull request adds a wildcard to each item in the exclude and also adds the docs directory to exclude. The sources list from windIO.egg-info/SOURCES.txt before and after are included below.

Current:

LICENSE
README.md
setup.py
test/plant/__init__.py
test/plant/test_resources_pywake.py
test/plant/test_system_pywake.py
test/plant/test_turbine.py
test/turbine/__init__.py
test/turbine/test_turbine.py
utils/__init__.py
utils/floris_utils.py
utils/foxes_utils.py
utils/pywake_utils.py
utils/topfarm_utils.py
utils/yml_utils.py
windIO/__init__.py
windIO.egg-info/PKG-INFO
windIO.egg-info/SOURCES.txt
windIO.egg-info/dependency_links.txt
windIO.egg-info/requires.txt
windIO.egg-info/top_level.txt
windIO.egg-info/zip-safe
windIO/plant/__init__.py
windIO/turbine/__init__.py

After this pull request:

LICENSE
README.md
setup.py
windIO/__init__.py
windIO.egg-info/PKG-INFO
windIO.egg-info/SOURCES.txt
windIO.egg-info/dependency_links.txt
windIO.egg-info/requires.txt
windIO.egg-info/top_level.txt
windIO.egg-info/zip-safe
windIO/plant/__init__.py
windIO/turbine/__init__.py
windIO/utils/__init__.py
windIO/utils/pywake_utils.py
windIO/utils/topfarm_utils.py
windIO/utils/yml_utils.py

Lastly, this pull request adds PyWake and TopFarm as dependencies in the setup config when requested. For a typical installation, these two packages are not installed, so this retains the current behavior. However, these two packages are installed when the test extras are requested:

pip install -e ".[test]"

@rafmudaf
Copy link
Collaborator Author

rafmudaf commented Sep 3, 2023

One more thing to note - I searched GitHub for which repositories are using the utils package to get a sense for the impact of changing the directory structure. The impact will be minimal, as you can see in these search results:

It's limited to this repository, and this pull request addresses those lines.

@SchmJo
Copy link

SchmJo commented Sep 7, 2023

I would like to include windio into the package foxes, and I think this pull request makes this a lot easier.

As a side note, a release to PyPi and conda-forge would even be better since it would allow to add windio as a dependency directly. Probably I am just being impatient, thanks for all the work!

@fzahle
Copy link
Collaborator

fzahle commented Sep 7, 2023

I generally approve of this change, since indeed the generic naming could conflict with many other packages.

Although it should not be addressed in this PR, I think we should separate the core windIO package from downstream frameworks that use it. Interfaces to these frameworks should be maintained in self-contained repos, that are then themselves responsible for checking for compatibility with changes in windIO.

@MMPe @bayc I think you added the utils code, so I leave it to you to decide on this one.

@rafmudaf
Copy link
Collaborator Author

rafmudaf commented Sep 7, 2023

Although it should not be addressed in this PR, I think we should separate the core windIO package from downstream frameworks that use it. Interfaces to these frameworks should be maintained in self-contained repos, that are then themselves responsible for checking for compatibility with changes in windIO.

@fzahle I agree, and I'll propose the change if the maintainers like it, too.

As a side note, a release to PyPi and conda-forge would even be better since it would allow to add windio as a dependency directly.

I'm also happy to add this infrastructure in another pull request, but I'll create a separate issue for this.

@kilojoules
Copy link
Collaborator

I completely agree. The constructors defined in the yaml reader are a crucial part of our defined schema. We should consider versioning the YAML schema as well as these associated constructors.

@ptrbortolotti
Copy link
Collaborator

thank you @rafmudaf ! I believe we are good to merge this PR?

@rafmudaf
Copy link
Collaborator Author

Yes, please!

@ptrbortolotti ptrbortolotti merged commit 2eeff57 into IEAWindTask37:master Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants