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

request_wpto_directional_spectrum improvements #166

Closed

Conversation

ryancoe
Copy link
Contributor

@ryancoe ryancoe commented Apr 4, 2022

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)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@ryancoe ryancoe changed the title datetime and dataset->dataarray request_wpto_directional_spectrum improvements Apr 4, 2022
@ssolson
Copy link
Contributor

ssolson commented Apr 11, 2022

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
Copy link
Contributor Author

ryancoe commented Apr 11, 2022

@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
Copy link
Contributor

ssolson commented Apr 11, 2022

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 ssolson changed the base branch from master to Develop February 1, 2023 15:14
This was referenced Feb 1, 2023
@ssolson
Copy link
Contributor

ssolson commented Feb 1, 2023

Closing in favor of #220

@ssolson ssolson closed this Feb 1, 2023
ssolson added a commit that referenced this pull request May 10, 2023
Supersedes #166 by @ryancoe to incorporate his suggested changes:

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

added gid to the metadata and added a description of gid to the docstring
General linting
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.

None yet

2 participants