search

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
50 stars 45 forks source link

request_wpto_directional_spectrum improvements #166

Closed ryancoe closed 1 year ago

ryancoe commented 2 years ago

Update the wave.io.hindcast.request_wpto_directional_spectrum function to:

  1. Use datetime objects
  2. Create a DataArray (not Dataset with weird variable naming)
ssolson commented 2 years ago

Hey @ryancoe I am less familiar with this module as I am not the author. The author Rebecca is expected to be on an extended leave anytime now. Your change to a Dataarray broke the way that multiyear arrays were working. Could you take a quick look and let me know if you would set up the multiyear storage differently based on your proposed change here?

======================================================================
ERROR: test_multi_loc (mhkit.tests.test_wave.TestWPTOhindcast)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/MHKiT-Python/MHKiT-Python/mhkit/tests/test_wave.py", line 1075, in test_multi_loc
    dir_multiyear = dir_multiyear.rename_vars({87:'87',58:'58'})
  File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/xarray/core/common.py", line 239, in __getattr__
    raise AttributeError(
AttributeError: 'DataArray' object has no attribute 'rename_vars'
ryancoe commented 2 years ago

@ssolson - I think test_wave.py has some odd subtleties that I didn't understand (and that caused the tests to run very slowly but pass on my machine).

  1. It runs different tests for different version of Python (I think this is to avoid making too many requests to the HSDS server that rex uses, which has a daily limit
  2. It uses long time.sleep statements to avoid making requests to HSDS at the same time.

Can you confirm or deny this interpretation ;) ?

Another question: Is there a reason the data variables have names like 87 and 58? I took this to be an oversight (i.e., 87 should be S for spectral density), but maybe I was missing something?

ssolson commented 2 years ago

1 and 2 are both correct. It's not ideal but it keeps us within limits of the free AWS requests.

I'm not sure on why the variable names are the way they are. @rpauly18 would be best to answer but she may be out of the office for an extended period. If it is returning spectral density then I would think that it should be called S but I am not sure how someone could accidentally use 87 there so I assume it is intentional.

ssolson commented 1 year ago

Closing in favor of #220