MHKiT-Software / MHKiT-Python

MHKiT-Python provides the marine renewable energy (MRE) community tools for data processing, visualization, quality control, resource assessment, and device performance.
https://mhkit-software.github.io/MHKiT/
BSD 3-Clause "New" or "Revised" License
47 stars 45 forks source link

Metocean module - WIND Toolkit #187

Closed akeeste closed 1 year ago

akeeste commented 1 year ago

@rpauly18 @ssolson This PR builds on a metocean module discussed in #167 by adding the IO capability for WIND Toolkit data, including vertical profiles of:

as well as surface values of

@Alex-McVey and I have not heard from Senu about desired metocean variables yet. However I have been working to build out IO functionality to pull in offshore WIND Toolkit data. This is going well so far as it is similar to the WPTO hindcast module. A new metocean example and tests for this module are in progress.

TODO items:

ssolson commented 1 year ago

Adam today I was looking at the wtk inputs and I think we should have a function to format the inputs for the user.

E.g. the inputs currently;

wtk_inputs = {'time_interval':'1-hour',
              'wind_parameters':['windspeed_10m','winddirection_10m'],
              'temp_parameters':['temperature_2m','temperature_20m',
                                 'temperature_40m','temperature_60m',
                                 'temperature_80m','temperature_100m',
                                 'temperature_120m','temperature_140m',
                                 'temperature_160m'],
              'year':[2018],
              'lat_lon':(40.748, -124.527)}

I think a quick function that returned the formatted list for wind direction and temperature should be included in the WIND toolkit. Something like:

wind_parameters = wind_toolkit.wind_parameters(windspeed=[10], winddirection=[10])
temp_parameters = wind_toolkit.temp_parameters([2,20,40,60,80,100,120,140,160])

wtk_inputs = {'time_interval':'1-hour',
              'wind_parameters': wind_parameters ,
              'temp_parameters': temp_parameters ,
              'year':[2018],
              'lat_lon':(40.748, -124.527)}

Alternatively, we could just have the inputs converted on the backend allowing the user to input more human-readable/ intuitive inputs like:

wtk_inputs = {'time_interval':'1-hour',
              'wind_parameters':[(10,10)],
              'temp_parameters':[2, 20, 40, 60, 80, 100, 120, 140, 160],
              'year':[2018],
              'lat_lon':(40.748, -124.527)}

Where the wind parameter speed and direction have been treated as a tuple and the temp parameters as a list. I don't expect either of the above to be the perfect solution but they are suggestions I was thinking of to make using the tool easier. What do you think?

akeeste commented 1 year ago

@ssolson Agreed, they can be tedious to type out and we can do something to make it easier. I prefer adding another small function to create variable for speed/direction/temperature profiles. That should require less checks for the parameters wtk_request_point_data and keep it a bit cleaner.

akeeste commented 1 year ago

Hi @ssolson

I have finished the various clean-up items and updated the notebook to reference each WIND Toolkit helper function. The only outstanding item is getting your thoughts on the updated plotting function, see your review comment that is not resolved.

Let me know if there's anything else we could improve, otherwise I'm pretty happy with things.

ssolson commented 1 year ago

Adam apologies. I thought the comments I had made were showing up on your most recent commit for me to review the change but they are on the old commit. I will resolve those comments again because I see the issue is resolved on your most recent commit.

akeeste commented 1 year ago

@ssolson Yes I should have pushed all those small changes. Thanks for checking.

A couple tests related to the wind_toolkit functions aren't passing. I'll double check this locally and see what's going on.

akeeste commented 1 year ago

This PR has some issues with the tests. The fact that different tests are failing in different jobs indicates to me that this may be an issue with the number of requests we are sending to the NREL developer server on AWS.

My tests pass locally, but on GitHub I see the following failures:

These are in addition to pip-windows-3.8/3.9, but I believe those are a different problem. @ssolson thoughts?

ssolson commented 1 year ago

@akeeste I believe the additional requests made to the AWS server are causing some of the issues. I re-ran the tests and all of the rex failures went away which confirms this. However, this PR is having issues with pip install on Windows 3.8/3.9 which other PRs are not currently having issues with e.g. #190 is currently passing without this issue. I cannot see any additional HDF5 or h5py changes in this PR which would cause these issues. This seems related to #181 but again I am not sure what has changed to cause pip install to fail. Adam are there any new packages or uses related to NetCDF/ hdf5/ h5py that I am currently missing in this PR?

ssolson commented 1 year ago

@akeeste I see that #190 was pointed against the master branch (I just pointed it to the Develop branch now) which did not include #181 which has those breaking pip changes. I am going to work on a fix today for this PR and #186 which if currently failing for the same reason.

akeeste commented 1 year ago

@ssolson

Adam are there any new packages or uses related to NetCDF/ hdf5/ h5py that I am currently missing in this PR?

At the top most level, no I do not import any new packages or have new custom uses of NetCDF/hdf5/h5py.

test_wind_toolkit does import netCDF4, but is not using it. I could clean up the imports there to ensure nothing is going wrong.

The only other place I see h5py being used is within the rex library. Both this PR and our WPTO Hindcast module call rex read and slice data from AWS. However h5py is not the default option to interact with the HSDS on AWS. Digging into the rex package, it is not immediately clear what the default option is. Even so, this case seems far too specific to cause the installation to fail.

ssolson commented 1 year ago

Apologies if my follow-up was not clear. I have confirmed now that the issue is the pip install/ the new version of NETCDF on the GitHub actions server. I am working on a fix in #194.

akeeste commented 1 year ago

Since #211 has fixed our HSDS issues, I'm redoing these tests. I'll confirm that the new HSDS calls for metocean data are not causing issues, then we can merge this PR.

ssolson commented 1 year ago

Looks like an easy fix for the failing tests FAILED ::TestWPTOhindcast::test_multi_loc - NameError: name 'np' is not defined

akeeste commented 1 year ago

Looks like an easy fix for the failing tests FAILED ::TestWPTOhindcast::test_multi_loc - NameError: name 'np' is not defined

Agreed. I'm not sure when/why I took that line out, but it's back in.

A couple last things--reviewing the test structure one more time I see that:

  1. WIND Toolkit tests are still in the main Actions workflow, not with the hindcast workflow or it's own workflow
  2. WIND Toolkit tests still use the previous hindcast test layout where certain function tests are split up by Python version (i.e. python 3.8 only uses test_multi_loc, 3.7 only uses test_multi_year)

The WIND Toolkit functions use the same rex and HSDS dependencies as the WPTO hindcasts. Breaking out a WIND Toolkit workflow might alleviate calling the server too much, but calling the tests for every python version will do the opposite. @ssolson after working on #211, do you have an intuition about what's best for us right now?

ssolson commented 1 year ago

@akeeste please see my PR at https://github.com/akeeste/MHKiT-Python/pull/1

ssolson commented 1 year ago

@akeeste I specified the exclusion of the hindcast tests from the typical tests as if they were in the new test module causing a coverage issue. I have fixed that mistake by excluding the correct files in my most recent PR here https://github.com/akeeste/MHKiT-Python/pull/2

akeeste commented 1 year ago

@ssolson I merged your PR https://github.com/akeeste/MHKiT-Python/pull/2 and also updated coveragehindcast.rc to refer to the correct files.

ssolson commented 1 year ago

@akeeste I believe merging akeeste#3 should get the tests passing.

akeeste commented 1 year ago

@ssolson moving my comment from https://github.com/akeeste/MHKiT-Python/pull/3 to this thread so that the discussion is easier to track:

The old versions of the files still remain in that commit on https://github.com/akeeste/MHKiT-Python/pull/3. We can add wind_toolkit.py and hindcast.py to mhkit.wave.io.hindcast, but they should also be removed from mhkit.wave.io. Doing this caused some issues with the tests because the paths in the relevant import calls and __init__.py files need to change. I made the required updates and am checking the tests again. The path for the two files are now: mhkit.wave.io.hindcast.hindcast.py and mhkit.wave.io.hindcast.wind_toolkit.py.

akeeste commented 1 year ago

@ssolson tests are passing! I will merge while there's no conflicts and issues.