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

Hindcast Data improvements #220

Closed ssolson closed 1 year ago

ssolson commented 1 year ago

This PR supersedes #166 by @ryancoe to incorporate his suggested changes:

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

  1. added gid to the metadata and added a description of gid to the docstring
  2. General linting
ssolson commented 1 year ago

@ryancoe in your PR #166 you requested that we return an xarray Dataset not a DataArray. Looking at your suggestion you had us squeeze the return data into a single Dataset as follows:

data = data_raw.to_xarray()
data = data_raw.to_xarray().to_array().drop('variable').squeeze()
data['time_index'] = pd.to_datetime(data.time_index)

What this was doing was dropping the variable names 57 and 87 which you asked a question about in the PR. These number refer to graphical identifiers or gids. So what you were doing here was collapsing wave spectra from two locations into one.

Is this useful or just confusion over what is being returned?

In this PR I have added gid to the metadata and added a description of gid to the docstring. Let me know if you think anything else should be improved in while I am here.

ssolson commented 1 year ago

Hey @akeeste could you take a look at this PR when you get a chance? It should be ready to go pending Ryan's feedback.

ryancoe commented 1 year ago

@ryancoe in your PR #166 you requested that we return an xarray Dataset not a DataArray...

I think I was jut confused about what I was dealing with. However, the location should ideally be an additional dimension. For example:

spectral_density = ...
ds = xr.Dataset(
    {
        "spectral_density": (["freq", "dir", "gid"], spectral_density),
    },
    ...
)
ssolson commented 1 year ago

@ryancoe in your PR #166 you requested that we return an xarray Dataset not a DataArray...

I think I was jut confused about what I was dealing with. However, the location should ideally be an additional dimension. For example:

spectral_density = ...
ds = xr.Dataset(
    {
        "spectral_density": (["freq", "dir", "gid"], spectral_density),
    },
    ...
)

Thanks Ryan. I'll take a look at your ideal suggestion over the next week to see how it fits in.

akeeste commented 1 year ago

Hi @ssolson

The improvements read nicely and should help out on this issue. Can you briefly document in this thread why you're removing boxplot.py and then can you rerun the tidal example? It's showing a couple of errors now.

ssolson commented 1 year ago

Hi @ssolson

The improvements read nicely and should help out on this issue. Can you briefly document in this thread why you're removing boxplot.py and then can you rerun the tidal example? It's showing a couple of errors now.

Thanks Adam. Yeah so to my knowledge we have never approved a python script to be added to the examples. I believe boxplot.py was added by accident. I will rerun the tidal example and look at Ryan's suggestion a little later this week and push some commits.

ssolson commented 1 year ago

Address #146 by allowing custom hsds path.