search

IEAWindTask37 / windIO

Apache License 2.0
19 stars 11 forks source link

Move utils package under windIO #18

Closed rafmudaf closed 8 months ago

rafmudaf commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

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

rafmudaf commented 8 months ago

Yes, please!