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

Dolfyn general updates #186

Closed jmcvey3 closed 1 year ago

jmcvey3 commented 1 year ago

Fixes for:

I was updating this branch from master, so there may be repeated commits in this list, though that shouldn't matter after resolving conflicts and merging.

jmcvey3 commented 1 year ago

Apparently I don't have "write access" to fix these conflicts

ssolson commented 1 year ago

Apparently I don't have "write access" to fix these conflicts

I think that should only occur if you are trying to push changes to the Develop branch. You should be able to resolve these conflicts on your local machine via git pull origin Develop Fix the conflicts git merge git push jmcvey dolfyn

jmcvey3 commented 1 year ago

@ssolson Two things: currently netcdf4 is limited by the environment.yml file to <1.5.8, and I just discovered that if installing with conda, (conda install -c conda-forge netcdf4), version 1.5.7 is the latest conda allows.

jmcvey3 commented 1 year ago

Having issues with netcdf compression encoding as well in tsdat, and the fundamental problem seems to be related to https://github.com/pydata/xarray/discussions/5709

jmcvey3 commented 1 year ago

@ssolson Tests are failing because xarray is searching for a package called bottleneck https://pypi.org/project/Bottleneck/ https://docs.xarray.dev/en/v0.8.0/installing.html. Can we add this package to the requirements list?

ssolson commented 1 year ago

Looks like it is an optional xarray package used to speed up working with NaNs which seems like it would be useful for DOLfYN. I checked out the repo and they seem active and have working tests for all current python versions. I think it should be good to add.

ssolson commented 1 year ago

Hey, James how goes this PR? Do you need any help?

jmcvey3 commented 1 year ago

Hi Sterling, this update should be good to go, once I'm sure the tests pass online. Related - is there a different process for updating documentation? I can put a pull request in for that as well.

jmcvey3 commented 1 year ago

@ssolson I do need help with fixing the "pip-windows-latest" tests. Should be the last thing.

ssolson commented 1 year ago

@jmcvey3 I am working on a fix.

jmcvey3 commented 1 year ago

Hi Adam,

Just left comments on the things you flagged. I removed a number of signal processing functions from velocity.py and tools/ because they're built-in to other packages like scipy (but weren't when dolfyn was originally written).

This pull request will add additional functionality to mhkit because only the basic processing functionality is available right now. I have a few more updates to add to this PR (or a new one) from working with Emily's Tanana River data and Levi's Cook Inlet data. I'm waiting on test data from Emily for those updates, and verification from Levi.

James

akeeste commented 1 year ago

@jmcvey3 Thanks for clarifying and helping track where those functions moved to. That resolves all of my comments. If this PR is otherwise ready to go and more large updates are coming, I think we should merge now and create a new PR for the next updates.

ssolson commented 1 year ago

@jmcvey3 could you merge the latest from Develop into this PR to see if it fixes your pip install CI issues (failing tests)?

jmcvey3 commented 1 year ago

Great, sounds good to me Adam