Ocean-Data-Lab / ooipy

Python library and demo code for processing and visualization of data from Ocean Observatories Initiative (OOI)
https://ooipy.readthedocs.io/en/latest/
MIT License
15 stars 8 forks source link

PR for #135 #147

Closed anishdixit-uw closed 1 year ago

anishdixit-uw commented 1 year ago

compute_spectrogram function changed to return xarray.DataArray

compute_psd_welch function changed to return xarray.DataArray

John-Ragland commented 1 year ago

Also, it looks like we'll need to change out unit test scripts to be able to handle the xarray DataArrays instead of the Spectrogram and PSD objects. These are located in ooipy/tests

anishdixit-uw commented 1 year ago

I think we're just left with changing the unit tests, how to do that? I don't see any test cases in ooipy/tests that are explicitly looking for Spectrogram or PSD objects being returned @John-Ragland

John-Ragland commented 1 year ago

From the errors in the unit test:

ooipy/tools/workflow.py:12: in <module>
    from ooipy.hydrophone.basic import Psd, Spectrogram
ooipy/hydrophone/basic.py:20: in <module>
    import xarray as xr
E   ModuleNotFoundError: No module named 'xarray'
=========================== short test summary info ============================
ERROR tests/test_request.py
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.52s ===============================
Error: Process completed with exit code 2.

Looks like it might be a dependency error with xarray. Also once we get ride of the Psd and Spectrogram objects the import statement above this will fail as well.

So we'll need to change setup.cfg. And we'll need to search through the whole repo and handle any cases where we work with spectrogram / psd objects and adjust that.

anishdixit-uw commented 1 year ago

Okay understood, let me see how I can edit setup.cfg and also comb through the repo to find any other functions utilizing Spectrogram and PSD classes

anishdixit-uw commented 1 year ago

Hey @John-Ragland , had a question, in the setup.cfg, I see 2 sections for package installations:

[options]
packages = find:
platforms = any
include_package_data = True
install_requires =
    obspy
    fsspec==2022.11.0
    aiohttp
    pandas
    numpy<1.22
    lxml
python_requires = >=3.7
setup_requires =
    setuptools_scm
py_modules = _ooipy_version

and

[isort]
known_first_party = ooipy
known_third_party = _ooipy_version,fsspec,matplotlib,numpy,obspy,pandas,requests,scipy,setuptools
multi_line_output = 3
include_trailing_comma = True
force_grid_wrap = 0
combine_as_imports = True
line_length = 100
skip =
    ooipy/__init__.py
    setup.py

So in which of these sections should I add Xarray? Or both?

John-Ragland commented 1 year ago

Hey @John-Ragland , had a question, in the setup.cfg, I see 2 sections for package installations:

[options]
packages = find:
platforms = any
include_package_data = True
install_requires =
    obspy
    fsspec==2022.11.0
    aiohttp
    pandas
    numpy<1.22
    lxml
python_requires = >=3.7
setup_requires =
    setuptools_scm
py_modules = _ooipy_version

and

[isort]
known_first_party = ooipy
known_third_party = _ooipy_version,fsspec,matplotlib,numpy,obspy,pandas,requests,scipy,setuptools
multi_line_output = 3
include_trailing_comma = True
force_grid_wrap = 0
combine_as_imports = True
line_length = 100
skip =
    ooipy/__init__.py
    setup.py

So in which of these sections should I add Xarray? Or both?

Honestly, I'm not sure. All of the packaging stuff is not my area of expertise. @lsetiawan, do you know? We are trying to add xarray as a dependency by editing the setup.cfg file.

lsetiawan commented 1 year ago

You need to add it under install_requires for listing dependency. The second section with [isort] is settings for isort a library that checks and sorts import statements. Not sure why this specification is needed. The packaging aspects will probably need to be updated in separate PR at some point.

...
install_requires =
    obspy
    fsspec==2022.11.0
    aiohttp
    pandas
    numpy<1.22
    lxml
    xarray
anishdixit-uw commented 1 year ago

Hey @John-Ragland , I have added Xarray to the dependencies and cleared other conflicts. All checks are passing and I think this PR is ready for merge.

John-Ragland commented 1 year ago

I think we can also remove the yml files for 3.7 and 3.8 in conda_envs/

Also, I'm not sure what we need to do to not have the 3.7 and 3.8 unit tests run.

John-Ragland commented 1 year ago

Also, since we're already doing a little work on the config file in this PR. It might be worth it to just go ahead and remove the outdated numpy requirement to be <= 1.22.

anishdixit-uw commented 1 year ago

@John-Ragland I made these changes:

  1. python version to be >=3.9
  2. Remove yml files for 3.7 and 3.8
  3. Removed numpys < 1.22 req

Now as you predicted, unit tests for 3.7 and 3.8 are failing, even I am unsure how we can stop them from running.. do you know where in the file system are we running these from?

John-Ragland commented 1 year ago

Looks like we need to edit this file: https://github.com/Ocean-Data-Lab/ooipy/blob/master/.github/workflows/unit_test.yml

John-Ragland commented 1 year ago

Let's add 3.11 while we're at it.

anishdixit-uw commented 1 year ago

@John-Ragland , did that, now the check for 3.7 and 3.8 are not running, but the check for 3.11 fails. Should I add a yml file in conda_envs for 3.11?

John-Ragland commented 1 year ago

@John-Ragland , did that, now the check for 3.7 and 3.8 are not running, but the check for 3.11 fails. Should I add a yml file in conda_envs for 3.11?

Yeah, I think you should be able to create mostly from copying the 3.10 yml

anishdixit-uw commented 1 year ago

Done, all checks passing @John-Ragland

John-Ragland commented 1 year ago

A couple more issues that we'll resolve with this PR

114 #140