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

Lint: Utils module #350

Merged
merged 43 commits into from
Nov 27, 2024
Merged

Lint: Utils module #350

merged 43 commits into from
Nov 27, 2024

Conversation

ssolson
Copy link
Contributor

@ssolson ssolson commented Sep 4, 2024

Improve PyLint Compliance and Update GitHub Actions Environment

  1. This PR makes the utils module PyLint compatible and enforces this compatibility through GitHub Actions.
  2. Updates the GitHub Actions testing environment to use the conda environment.yml file for consistency.
  3. Updates the folium map calls in the Pacwave example to align with the latest folium API changes.

@ssolson ssolson self-assigned this Sep 4, 2024
@ssolson ssolson added utils module Clean Up Improve code consistency and readability labels Sep 4, 2024
mhkit/river/io/usgs.py Show resolved Hide resolved
mhkit/river/io/usgs.py Show resolved Hide resolved
mhkit/tests/utils/test_cache.py Show resolved Hide resolved
mhkit/tests/utils/test_cache.py Show resolved Hide resolved
mhkit/tests/utils/test_cache.py Show resolved Hide resolved
mhkit/wave/io/hindcast/wind_toolkit.py Show resolved Hide resolved
mhkit/wave/io/ndbc.py Show resolved Hide resolved
mhkit/wave/io/ndbc.py Show resolved Hide resolved
mhkit/wave/io/ndbc.py Show resolved Hide resolved
.github/workflows/pylint.yml Show resolved Hide resolved
@ssolson ssolson marked this pull request as ready for review September 6, 2024 19:09
@ssolson ssolson requested a review from akeeste September 6, 2024 19:09
.github/workflows/main.yml Show resolved Hide resolved
shell: bash -l {0}
run: |
conda install numpy cython pip hdf5 libnetcdf cftime netcdf4 --strict-channel-priority
pip install -e . --force-reinstall
conda env create -f environment.yml
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use the MHKiT conda env from enviroment.yml file

Comment on lines +79 to +80
conda activate mhkit-env
conda install -y pytest coverage coveralls
Copy link
Contributor Author

Choose a reason for hiding this comment

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

install development specific packages

shell: bash -l {0}
run: |
conda install numpy cython pip pytest hdf5 libnetcdf cftime netcdf4 coverage --strict-channel-priority
pip install -e . --force-reinstall
conda env create -f environment.yml
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use the MHKiT conda env from enviroment.yml file

shell: bash -l {0}
run: |
conda activate mhkit-env
conda install -y pytest coverage coveralls nbval jupyter utm folium
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra packages this time for the notebook examples

@@ -1,4 +1,4 @@
name: myenv
name: mhkit-env
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set env name to mhkit-env

Comment on lines +23 to +27
- pecos>=0.3.0
- notebook
- matplotlib>=3.9.1
- fatpack
- nrel-rex
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solve everything using conda (no pip)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the folium package updated and required a modification to the pacwave map

@ssolson
Copy link
Contributor Author

ssolson commented Nov 19, 2024

@akeeste this PR is ready for review

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to install mhkit in this file at the end? It saves the installer a step? Not sure if you are not installing mhkit here on purpose?

Copy link
Contributor Author

@ssolson ssolson Nov 20, 2024

Choose a reason for hiding this comment

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

It would be possible and I am not installing it on purpose because it would not make sense for testing.

If MHKiT was included in the .env file then that would install the current conda release vs the changes in the repo we are trying to test.

My thoughts on how users would interact with the .env file:

  • Anyone simply wanting to use MHKiT I expect to use our release via conda install mhkit .
  • Anyone using the .env file I expect is doing development in the repo.

Copy link
Contributor

@akeeste akeeste Nov 20, 2024

Choose a reason for hiding this comment

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

That makes sense to me. I think our documentation could use an update on clearly delineating the use of the different install methods (environment.yml, conda install, editable) for different use cases (developer, user, user who wants notebooks too). I can take that on in the docs repo as I make other updates

  • Anyone using the .env file I expect is doing development in the repo.
    So the developer installation workflow would be:
  • create conda environment with environment.yml
  • pip install -e . for a editable MHKiT install

Also pip install -e . is being deprecated and we'll need a new method soon

Copy link
Contributor

@akeeste akeeste left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good @ssolson. A few clarifying questions throughout

.github/workflows/main.yml Show resolved Hide resolved
Copy link
Contributor

@akeeste akeeste Nov 20, 2024

Choose a reason for hiding this comment

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

That makes sense to me. I think our documentation could use an update on clearly delineating the use of the different install methods (environment.yml, conda install, editable) for different use cases (developer, user, user who wants notebooks too). I can take that on in the docs repo as I make other updates

  • Anyone using the .env file I expect is doing development in the repo.
    So the developer installation workflow would be:
  • create conda environment with environment.yml
  • pip install -e . for a editable MHKiT install

Also pip install -e . is being deprecated and we'll need a new method soon

mhkit/utils/cache.py Outdated Show resolved Hide resolved
mhkit/utils/type_handling.py Show resolved Hide resolved
@ssolson
Copy link
Contributor Author

ssolson commented Nov 26, 2024

@akeeste I added the docstring back that I mistakenly deleted.

Assuming the tests pass is this ready to merge?

@akeeste
Copy link
Contributor

akeeste commented Nov 26, 2024

@ssolson One outstanding question on the module docstring here, but that is looking towards a larger standardization of our docstrings across MHKiT.

If tests pass, let's merge

@ssolson ssolson merged commit 27114e7 into MHKiT-Software:develop Nov 27, 2024
46 checks passed
@ssolson ssolson deleted the lint_utils branch November 27, 2024 02:59
@ssolson ssolson mentioned this pull request Dec 4, 2024
@ssolson ssolson mentioned this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Up Improve code consistency and readability utils module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants