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

MHKiT v0.8.0 #307

Closed
wants to merge 65 commits into from
Closed

MHKiT v0.8.0 #307

wants to merge 65 commits into from

Conversation

ssolson
Copy link
Contributor

@ssolson ssolson commented Apr 24, 2024

MHKiT v0.8.0

We're excited to announce the release of MHKiT v0.8.0, which brings a host of new features, enhancements, and bug fixes across various modules, ensuring compatibility with Python 3.10 and 3.11, and introducing full xarray support for more flexible data handling. Significant updates in the Wave and DOLfYN modules improve functionality and extend capabilities.

Python 3.10 & 3.11 Support

MHKiT now supports python 3.10 and 3.11. Support for 3.12 will follow in the next minor update.

Wave Module

Enhancements:

Automatic Threshold Calculation for Peaks-Over-Threshold: We've introduced a new feature that automatically calculates the "best" threshold for identifying significant wave events. This method, originally developed by Neary, V. S., et al. in their 2020 study, has now been translated from Matlab to Python, enhancing our existing peaks-over-threshold functionality.

Wave Heights Analysis: A new function, wave_heights, has been added to extract the heights of individual waves from a time series. This function uses zero up-crossing analysis to accurately measure wave heights, improving upon our previous methods which only provided the maximum value between up-crossings.

Enhanced Zero Crossing Analysis: Building on the above, the zero crossing code previously embedded in global_peaks has been isolated into a helper function. This modular approach not only refines the codebase but also supports new functionalities such as calculating wave heights, zero crossing periods, and identifying crests.

Bug Fixes:

Contour Sampling Error in Wave Contours: A bug identified in mhkit.wave.contours.samples_contour has been resolved. The issue occurred when period samples defined using the maximum period resulted in values outside the interpolation range of the contour data. This was corrected by ensuring that all sampling points are within the interpolation range and adjusting the contour data selection process accordingly.

Xarray Support

MHKiT functions now fully support the use of xarray for passing and returning data.

DOLfYN

Thanks to the many user contributions and users of MHKiT the DOLFYN module include a significant number of enhancements and bug fixes.

Enhancements:

Altimeter Support: Enhanced the Nortek Signature Reader to add capability for reading ADCP dual profile configurations.

Data Handling Improvements: Introduced logic to skip messy header data that can accumulate during measurements collected via Nortek software on PCs and Macs.

Instrument Noise Subtraction: Added a function to subtract instrument noise from turbulence intensity estimation using RMS calculations, providing results that differ by approximately 1% from the existing standard deviation-based "I" property.

Improved File Handling: Updates for RDI files to handle changing "number of cells" and variable "cell sizes," which are now bin-averaged into the largest cell size.

Bug Fixes:

Power Spectra Calculation: Fixed a bug where a given noise value was not being subtracted from the power spectra, and noise was inadvertently added as an input to dissipation rate calculations.

Improved Header Handling: Allowed RDI reader to skip junk headers effectively.

Nortek Reader C Types Update: Adjusted C types in the Nortek reader to handle below-zero water temperatures and to allow pitch and roll values to go negative.

River & Tidal: D3D

Added limits to variable_interpolation and added 3 array input capability to create_points

Developer Experience

Black formatting

Black formatting is now enforced on all MHKiT files. This ensures consistent formatting across the MHKiT package.

Linting & Type Hints

MHKiT is in the process of enforcing pylint and adding type hints to all functions. Currently this has been achieved and is enforced in the Loads and Power modules.

CI/CD

This release introduces significant reduction in testing time for development. This is achieved by reducing the number of tests for pulls against the develop branch and only running hindcast test when changes are made to it. A bug in the hindcast CI was fixed which only ran on changes to the hindcast tests instead of the hindcast module.

Clean Up

MHKiT fixed an implementation error where functions used assert instead of built in errors for type and value checking. Additionally these PRs removed unused files, fixed typos, and created an argument which allows users to run CDIP API calls silently.

ssolson and others added 30 commits October 12, 2023 10:45
Use caching by default & reduce testing time
* run hindcast only on change

* always run hindcast on changes to master
* allow latest versions from all requirements

* add python 3.10 and 3.11 

* only test ubuntu on pulls to dev; mac & Windows on pull to master

* use actions/upload-artifact@v3; setup-python@v3; download-artifact@v3
* automatic Hs threshold

* Revert "automatic Hs threshold"

This reverts commit 4477580.

* automatic Hs threshold

* simplify & include MATLAB example for debugging

* fix independent storms

* Update mhkit/loads/extreme.py

Co-authored-by: Adam Keester <[email protected]>

* Update mhkit/loads/extreme.py

Co-authored-by: Adam Keester <[email protected]>

* Update mhkit/loads/extreme.py

Co-authored-by: Adam Keester <[email protected]>

* Update mhkit/loads/extreme.py

Co-authored-by: Adam Keester <[email protected]>

* cleanup

* Update mhkit/tests/loads/test_loads.py

* Update mhkit/tests/loads/test_loads.py

Update threshold test value one more time

* Update extreme.py

* break out nested function, consolidate scipy imports

---------

Co-authored-by: ssolson <[email protected]>
Co-authored-by: Adam Keester <[email protected]>
Co-authored-by: akeeste <[email protected]>
* Add test for loads.extreme.global_peaks function

The loads_extreme.global_peaks function was previously missing a test.

The test uses a simple function which can be independently analysed. The results of global_peaks and the independent analysis are then compared.

* Introduce upcrossing module

Previously there was no general means of performing an upcrossing analysis. The load.extreme.global_peaks function could only calculate peaks.

The module provides some common methods but also the ability for the user to define their own function over the zero crossing points.

* Implement loads.extreme.global_peaks in terms of upcrossing module

With the recent addition of the upcrossing module, we can implement the loads.extreme.global_peaks function using it.

* minor linting, module docstring, update parameter validation

* fix upcrossing docstring

* move upcrossing module to utils

* update upcrossing import in example

* fix last upcrossing docstring and move test to test/utils

* update description in the upcrossing notebook

* update import of upcrossing into test_upcrossing

* typo in test file

* final typo fix

---------

Co-authored-by: akeeste <[email protected]>
* remove ci folder

* update README installation instructions

* remove figures folder

* remove pypirc
* add silent kwarg to request_parse_workflow and get_netcdf_variables

* describe optional use of silent in the cdip example

* fix typo

* fix netcdf typo
* loads/extreme convert asserts to errors

* loads/general convert asserts to errors

* loads/graphics convert asserts to errors

* power module convert asserts to errors

* loads module, convert try-except validation to errors

* utils module, convert asserts to errors

* dolfyn module, convert asserts to errors

* tidal module, convert asserts to errors

* river module, convert asserts to errors

* wave hindcast, replace asserts with errors

* wave/io module, convert asserts to errors

* wave module, convert asserts to errors

* rename reserved variable min in plot_directional_spectrum

* fix miscellaneous typos

* catch new error type

* fix test logic and parameter name

* minor review comments, fix f strings, fix messages, etc

* list correct types in error messages, f strings, etc

* standardize error messages for optional parameters

* Apply suggestions from ssolson's 2nd review
…278)

* check is samples min or max included in the contour half

* ensure requested samples are within range of contour values
* Added limits to variable_interpolation and added 3 array input capability to create_points


Co-authored-by: Browning <[email protected]>
Co-authored-by: ssolson <[email protected]>
* loads/general

* loads/extreme conversion to xarray

* test and bugfix for loads/general/bin_statistics

* fix bug in bin_statistics where std=0

* correct bin_statistics test data

* update bin_statistics test

* fix dimension name in mler_wave_amp_normalize

* formatting fixes

* update return types, add optional dimension argument

* add Series and DataArray to type check for mler_coefficients

* update argument and docstring for time_dimension

* update dimension variables to time_dimension

* rename time_dimension to frequency_dimension where required
* #277 
* #284 - Fix window check
* Test updates plus compression bugfix
* power/quality add basic formatting

* initial conversion or power/quality submodule

* add xarray tests for power.quality

* fix variable assignment in power.quality

* power.characteristics add basic formatting for xr conversion

* update error messages

* finish converting power.quality to xarray

* fix spaces/formating in docstrings

* clean up conversion of inputs from pandas to xr.dataset

* add tests for xarray

* update handling of timestamps in power.characteristics for xr

* fix length in power.quality.harmonics

* fix length in power.quality.harmonics, again

* add frequency_dimension and time_dimension arguments

* remove old imports

* remove obsolete argument from THCD tests

* type check on to_pandas

* add type and value checks for time_dimension and frequency_dimension

* make grid_freq checks f strings that return the incorrect value

* add frequency_dimension valueError

* add formal docstring to _convert_to_dataset

* add line_to_line type check

* restore old naming convention to ac_power_three_phase

* update example call to THCD

* return hard coded test answers to being recalculated
- black formatting
- GitHub actions black
- new API key added for NREL's HSDS server
- `mhkit.tests.river.test_io` broken out into `mhkit.tests.river.test_io_d3d` and `mhkit.tests.river.test_io_usgs`
- added functional tests and error handing tests to `mhkit.tests.dolfyn.test_tools`, `mhkit.tests.mooring.test_mooring`
- added error handling tests for `mhkit.tests.river.test_resource`, `mhkit.tests.wave.io.hindcast.test_wind_toolkit`, `mhkit.tests.wave.test_contours`, `mhkit.tests.loads.test_loads`
@ssolson ssolson marked this pull request as ready for review April 24, 2024 15:54
@ssolson ssolson requested a review from akeeste April 24, 2024 15:54
@ssolson ssolson mentioned this pull request Apr 24, 2024
* fix type error message in power.quality

* convert wave.performance to xarray

* remove extraneous variables from test_performance

* wave/resource 90 percent converted to xarray

* speed up conversion to dataset

* wave.resource conversion complete

* allow xarray input to wave.contours functions

* allow xarray in wave.graphics

* minor cleanup

* update type handling in wave_number and depth_regime

* update error messages in convert_to_dataarray

* test_wave_metrics passing

* correct typo in graphics

* wave tests passing except frequency binned surface_elevation calls

* fix bug/memory error with surface_elevation frequency bins

* black formatting

* allow xarray output in wave.io.cdip

* allow xarray output from wave.io.wecsim

* make utils function to convert nesed dicts of dataframes to xarray

* allow xarray input/output in wave.io.swan

* allow xarray input/output in wave.io.ndbc

* add xarray test for test_wecsim.py

* add xarray test in test_cdip

* update dataframe to dataarray case in utils.type_handling

* add xarray test to test_ndbc

* add xarray tests to test_swan

* black formatting

* fix single value dataframe to dataarray conversion

* black formatting

* update docstring in type_handling

* update description of to_pandas flag when returning a dict of pd or xr

* update ndbc docstrings

* move TODO from comment to an issue

* update error message

* update output types for wavelength

* add frequency_dimension and time_dimension kwargs

* add check for shape of phases and S in surface_elevation

* black formatting

* pylint suggestion in power.characteristics

* fix cdip example

* allow xarray output in wave.io.hindcast functions
@ssolson
Copy link
Contributor Author

ssolson commented Apr 24, 2024

@akeeste it looks like #302 introduced some xarray issues we did not catch in review related to the hindcast module. I need to look into why these tests were not run if we changed the hindcast module.

@akeeste
Copy link
Contributor

akeeste commented Apr 24, 2024

Hey @ssolson I added one quick fix that I did not catch in #302. See comment here .

So I did add one minor change to the hindcast module, but the tests passed on my last commit. Apologies I should've look closer, I assumed the hindcast tests ran on that commit, but they did not. The tests is failing now because I updated a flag name from as_xarray to to_pandas to match the rest of our xarray work. I'll submit a quick fix for that into dev, see #310

* restore correct default for pandas/xarray output

* update as_xarray flag to to_pandas in hindcast tests
@ssolson
Copy link
Contributor Author

ssolson commented Apr 25, 2024

It seems like the hindcast tests are getting stuck in between the wave and wind test and then failing on a timeout.
image

There is no error so I am going to need to just try a actions change in a new PR and see if that fixes it.

image

This PR addresses 2 issues with the hindcast tests:

* On PRs tests were only being run on changes to hindcast test files.
  - Fixed to run on changes to hindcast test and module files.
* The prepare cache job was failing due to 6 hour job limit.
  - Fixed by breaking out each hindcast cache test into its own job.

* Additionally this PR updates action packages to the latest version where possible.
@ssolson
Copy link
Contributor Author

ssolson commented Apr 29, 2024

Okay new problem. Conda on mac now fails but I believe it is just the Actions package we are using. Looking for a fix and opening a new PR.

image

image

@simmsa
Copy link
Contributor

simmsa commented Apr 29, 2024

@ssolson, actions running macos-latest appear to be using an arm64 image of macos. This was causing trouble with MHKiT-MATLAB tests. Dropping down to macos-13 uses an image without arm in the name and fixed our issues with failing tests. We did experiment with setting the os to macos-14 but that appears to use an arm image as well.

conda-incubator/setup-miniconda@v3 to install conda.
@ssolson
Copy link
Contributor Author

ssolson commented Apr 30, 2024

@ssolson, actions running macos-latest appear to be using an arm64 image of macos. This was causing trouble with MHKiT-MATLAB tests. Dropping down to macos-13 uses an image without arm in the name and fixed our issues with failing tests. We did experiment with setting the os to macos-14 but that appears to use an arm image as well.

Thanks @simmsa if this does not fix the issue I will force a specific mac os build

@akeeste
Copy link
Contributor

akeeste commented May 2, 2024

Thanks @ssolson I added a couple items to the release notes above and they are good now. Do you need any support getting our tests passing?

@ssolson
Copy link
Contributor Author

ssolson commented May 2, 2024

Thanks @ssolson I added a couple items to the release notes above and they are good now. Do you need any support getting our tests passing?

Thanks @akeeste. No mostly the issue is that the fix for one build introduces an issue for a different build. I am not stuck just juggling options to get this all working.

@simmsa
Copy link
Contributor

simmsa commented May 2, 2024

@ssolson these are my opinions based on the MHKiT-MATLAB Actions.

The latest prepare-wave-hindcast-cache and prepare-wind-hindcast-cache build failures appear to be caused by Actions defaulting to python 3.12. These could probably be fixed by setting the miniconda python-version: ${{ env.PYTHON_VER }}

The conda-windows jobs failures seem to match these test failures in MHKiT-MATLAB. This seemed to be due to the capital letters in the conda environment name. We now use lowercase letters in the conda environment name and no longer have the ImportError: DLL load failed issue. I couldn't find any info about this specific error with Actions/Window/Anaconda so this may not fix this failure.

The pip-macos-latest 3.8 job failure is most likely due to the arm image of MacOS. Possible solutions are brew install netcdf or using macos-13

@ssolson
Copy link
Contributor Author

ssolson commented May 2, 2024

@ssolson these are my opinions based on the MHKiT-MATLAB Actions.

The latest prepare-wave-hindcast-cache and prepare-wind-hindcast-cache build failures appear to be caused by Actions defaulting to python 3.12. These could probably be fixed by setting the miniconda python-version: ${{ env.PYTHON_VER }}

The conda-windows jobs failures seem to match these test failures in MHKiT-MATLAB. This seemed to be due to the capital letters in the conda environment name. We now use lowercase letters in the conda environment name and no longer have the ImportError: DLL load failed issue. I couldn't find any info about this specific error with Actions/Window/Anaconda so this may not fix this failure.

The pip-macos-latest 3.8 job failure is most likely due to the arm image of MacOS. Possible solutions are brew install netcdf or using macos-13

Thanks @simmsa the issues you are addressing here are a bit outdated and I have solved them in #318. Like I mentioned to @akeeste the solutions created new issues which I think I have now solved by using the coveralls actions. Hope to merge those changes soon.

@simmsa
Copy link
Contributor

simmsa commented May 2, 2024

@ssolson, my bad! Thanks for pointing me in the right direction!

- Uses python version correctly
- Installs MHKiT dependencies in conda build 
- Use coverage actions instead of coverage CLI 
- Uses an updated coverage version with lcov support
- Adds an `environment.yaml` file for conda environment install 
- Special case for MacOS-latest (macos 14)  and Py 3.8
- CI test job `set-os` logic modified to now run all OS on push to `develop`
@ssolson ssolson mentioned this pull request May 4, 2024
- `hindcast-calls` job now uses the environment.yaml file & forces the correct coverage version as added in #317 
- Wind cache waits on wave cache completion
@ssolson
Copy link
Contributor Author

ssolson commented May 6, 2024

The tests are passing there is just an edge case causing the failure on one of the 2 identical runs.

Here is how:

  1. We create a run on push to develop and PR to master which means we start 2 identical jobs when closing a PR to develop currently
    a. Passing push - https://github.com/MHKiT-Software/MHKiT-Python/actions/runs/8959832115
    b. Failing PR - https://github.com/MHKiT-Software/MHKiT-Python/actions/runs/8959831356/attempts/1
  2. The Wind hindcast job above fails because there is another hindcast call job hitting the API at the same time
  3. Resubmitting a job currently fails I think because I had non-unique flag-names which I am addressing in CI: Coveralls flag-name #320
    a. https://github.com/MHKiT-Software/MHKiT-Python/actions/runs/8959831356/attempts/2

This can be merged to master as is because the tests will now pass and no changes I make will impact the users. We could wait for #320. We could also get around this edge case by creating a v0.8.0 branch and creating a PR against master on that branch.

@simmsa
Copy link
Contributor

simmsa commented May 6, 2024

@ssolson are we at the point that we should update the version number in __init__.py? Not sure when you all typically change this.

@ssolson
Copy link
Contributor Author

ssolson commented May 6, 2024

@ssolson are we at the point that we should update the version number in __init__.py? Not sure when you all typically change this.

Thanks! Usually I like to merge to master, publish the package and then have a follow on PR after I realize my mistake.

added in 328ad1e

@ssolson ssolson mentioned this pull request May 6, 2024
@ssolson
Copy link
Contributor Author

ssolson commented May 6, 2024

I am going to close this PR and supersede with #321 to avoid the edge case issues above.

@ssolson ssolson closed this May 6, 2024
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.

7 participants