Improve flexibility and robustness of Acoustics Module - #433
Conversation
…PSDs are calculated
|
Looks like some remaining dependency issues related to timestamps |
|
@jmcvey3, from my perspective you are not obligated to have all of the tests passing here to merge this into develop. Do you have any thoughts/preferences here? I'm guessing all of the tests pass locally for you, so let us know what makes sense. This is likely going to continue to be an issue moving forward with MHKiT, so I wonder if there is better way for the tests to only check "things that matter", and maybe only run pytest on the modules where the code has changed. It is def appreciated to get the tests working here, but I think this is more of the tests are broken in general vs this PR's tests are not passing... Happy to take any feedback here also. The actions have a lot of false positives IMO. I like the CI/CD idea in general, but there are also a lot of ways that this impedes actual development, so it seems like we need to find a balance... |
|
Well.... I tracked down the not-pretty bug in the cdip.py file and fixed that since it was a recurring issue. In the process, I cleaned up the dependencies in the conda environment file, so now hopefully these tests will run faster now that they don't have to install dependencies twice each time. I most likely broke the pip installation because of the h5-related libraries, but we'll see whenever those tests finish. |
Perhaps, but these have been frustrating for a while now, and I need this PR merged. I'm realizing what I'm running through now may have been why the packaging was difficult for the 1.0 release |
Do you just need this merged into develop? I think if you are happy with Adam's review and your tests are passing locally it is fine to merge this? I pulled your branch and all of the tests in the acoustics module pass locally for me in a fresh virtual environment: What do you think? I don't want to set a bad precedent, but you are not merging this into main, you are merging into develop, and that seems like what the develop branch is for. My own opinion is that some of the changes you are making in this PR are out of scope. That is fine and I get it, but I think we need to be careful about doing too many things to "appease the tests". |
Sure sure. I can create a new branch and a new PR for these changes and then rebase this PR back to just the acoustics code. I'll redo the "black" commit too so it's just editing the acoustics files as well. |
|
I agree with your first comment @simmsa , if the relevant tests are passing I think we should merge PRs as is to get that feature taken care of and then move to fixing the tests. Ideally, we'd do the test fixes first into develop and pull the updates into the feature PR (such as this one)--but none of us have the bandwidth to do so very quickly. @jmcvey3 For this particular PR, I will support whatever will be most efficient, whether that's merging as is, putting another fix here for the last test, or rebasing the test fixes into a new PR |
d965fc0 to
ffc3f65
Compare
Sounds good. I've rebased here and created a new PR for the other changes |
# MHKiT-Python v1.1.0 ## Additions * Acoustics: Add millicdecade and WISPR instrument support * Added millidecade spectral conversion * Added a WISPR hydrophone reader * Added a voltage-based `export_audio` resampling option * Refactored band-averaging to avoid losing information at frequency-band boundaries * #447 * Author: @jmcvey3 * Reviewer: @simmsa * DOLfYN: Add [Nortek Aquadopp](https://www.nortekgroup.com/oceanography/aquadopp-series) ADCP support * Added DOLfYN support for reading Aquadopp instruments * Cleaned Nortek parsing code * Simplified handling of the non-cabled ADV orientation flag * #434 * Author: @jmcvey3 * Reviewer: @akeeste * Examples: Add ADCP waves example: `example/adcp_waves_example.ipynb` * Added an example notebook showing how to ingest and analyze wave measurements from a dual-profile Nortek Signature 250 deployment at PacWave. * #430 * Author: @jmcvey3 * Reviewer: @akeeste ## Improvements ### Acoustics - Improved flexibility and robustness of the Acoustics module - Added a configurable FFT length for sound pressure PSDs - Renamed bin/windowing attributes for clarity - Fixed an incorrectly signed gain correction - #433 - Author: @jmcvey3 - Reviewer: @akeeste ### DOLfYN - Refactored PSD calculations to use `scipy.signal.welch` - Replaced DOLfYN's custom Welch-like PSD implementation (built each segment from bin slices, could not overlap FFT segments) with `scipy.signal.welch` - Removed the bin-based segment structure and added a `step` argument to control overlap - Dropped float32 casts in some functions in favor of float64 - Removed `fft.py` and renamed `tools/misc.py` to `tools.py` - Breaking Changes: - NaNs are no longer tolerated in PSD calculations - DOLfYN defaults to 0% overlap to preserve existing array shapes - Acoustics defaults to 50% overlap - PSD output time dimension renamed to `time_psd` - #452 - Author: @jmcvey3 - Reviewer: @akeeste, @simmsa - Improved handling of "averaged" profiles - Fixed handling of Nortek Signature dual-profile ADCP data by defaulting to "_avg" velocity variables when untagged ones are absent. - #430 - Author: @jmcvey3 - Reviewer: @akeeste - Critical PSD bugfix - Fixed a bug where individual FFTs received a 50% overlap twice, which corrupted the first and last spectrum of a timeseries. - #430 - Author: @jmcvey3 - Reviewer: @akeeste ### Examples - Added histograms to ADCP example - #448 - Author: @browniea - Reviewer: @akeeste ### River/IO - Fixed Qhull interpolation and D3D coordinate-system errors - #448 - Author: @browniea - Reviewer: @akeeste - Fixes: #442, #444 - Delft3D module updates - Added new Delft3D coordinate names - Added a new grid-convergence-index calculation function - Added support for xarray/netCDF4 input in the D3D module - #428 - Author: @browniea - Reviewer: @akeeste ### Wave - NDBC Directional Wave Units - Fixed NDBC directional wave spectrum output to return degrees instead of radians - Updated polar plots so 0 deg is at the top and increases clockwise - #437 - Author: @jmcvey3 - Reviewer: @akeeste - Fixes: #427 ### Wave/Hindcast - Added a `hindcast_guard` exception-handling decorator (`hindcast_exceptions.py`) that surfaces a clear error on HSDS request failures, distinguishing the known NLR HSDS outage (#450) from other failures - #449 - Author: @simmsa - Reviewer: @akeeste ## Maintenance - Added Python 3.13 support - #445 - Author: @simmsa - Reviewer: @akeeste - Fixes: #441 - Added [pandas 3](https://pandas.pydata.org/community/blog/pandas-3.0.html) Support - Updated the pandas dependency to allow pandas 3.x - Added a compatibility shim for NDBC missing-value handling related to pandas 3 object to String dtype api changes - Fixed a deprecated period alias in tests - #443 - Author: @simmsa - Reviewer: @akeeste - Fixes: #440 - Updated GitHub Actions CI, expanded installation/developer documentation, refreshed dev environment, and trimmed dependencies - Refactored optional dependencies - Standardized conda/conda-forge environment builds - Scoped black linting to changed files - #436 - Author: @simmsa - Reviewer: @akeeste - Update `rex` dependency to target pypi package to [`NLR-rex[hsds]>=0.5.0`](https://pypi.org/project/NLR-rex/) - #449 - Author: @simmsa - Reviewer: @akeeste
# MHKiT-Python v1.1.0 ## Additions * Acoustics: Add millicdecade and WISPR instrument support * Added millidecade spectral conversion * Added a WISPR hydrophone reader * Added a voltage-based `export_audio` resampling option * Refactored band-averaging to avoid losing information at frequency-band boundaries * MHKiT-Software#447 * Author: @jmcvey3 * Reviewer: @simmsa * DOLfYN: Add [Nortek Aquadopp](https://www.nortekgroup.com/oceanography/aquadopp-series) ADCP support * Added DOLfYN support for reading Aquadopp instruments * Cleaned Nortek parsing code * Simplified handling of the non-cabled ADV orientation flag * MHKiT-Software#434 * Author: @jmcvey3 * Reviewer: @akeeste * Examples: Add ADCP waves example: `example/adcp_waves_example.ipynb` * Added an example notebook showing how to ingest and analyze wave measurements from a dual-profile Nortek Signature 250 deployment at PacWave. * MHKiT-Software#430 * Author: @jmcvey3 * Reviewer: @akeeste ## Improvements ### Acoustics - Improved flexibility and robustness of the Acoustics module - Added a configurable FFT length for sound pressure PSDs - Renamed bin/windowing attributes for clarity - Fixed an incorrectly signed gain correction - MHKiT-Software#433 - Author: @jmcvey3 - Reviewer: @akeeste ### DOLfYN - Refactored PSD calculations to use `scipy.signal.welch` - Replaced DOLfYN's custom Welch-like PSD implementation (built each segment from bin slices, could not overlap FFT segments) with `scipy.signal.welch` - Removed the bin-based segment structure and added a `step` argument to control overlap - Dropped float32 casts in some functions in favor of float64 - Removed `fft.py` and renamed `tools/misc.py` to `tools.py` - Breaking Changes: - NaNs are no longer tolerated in PSD calculations - DOLfYN defaults to 0% overlap to preserve existing array shapes - Acoustics defaults to 50% overlap - PSD output time dimension renamed to `time_psd` - MHKiT-Software#452 - Author: @jmcvey3 - Reviewer: @akeeste, @simmsa - Improved handling of "averaged" profiles - Fixed handling of Nortek Signature dual-profile ADCP data by defaulting to "_avg" velocity variables when untagged ones are absent. - MHKiT-Software#430 - Author: @jmcvey3 - Reviewer: @akeeste - Critical PSD bugfix - Fixed a bug where individual FFTs received a 50% overlap twice, which corrupted the first and last spectrum of a timeseries. - MHKiT-Software#430 - Author: @jmcvey3 - Reviewer: @akeeste ### Examples - Added histograms to ADCP example - MHKiT-Software#448 - Author: @browniea - Reviewer: @akeeste ### River/IO - Fixed Qhull interpolation and D3D coordinate-system errors - MHKiT-Software#448 - Author: @browniea - Reviewer: @akeeste - Fixes: MHKiT-Software#442, MHKiT-Software#444 - Delft3D module updates - Added new Delft3D coordinate names - Added a new grid-convergence-index calculation function - Added support for xarray/netCDF4 input in the D3D module - MHKiT-Software#428 - Author: @browniea - Reviewer: @akeeste ### Wave - NDBC Directional Wave Units - Fixed NDBC directional wave spectrum output to return degrees instead of radians - Updated polar plots so 0 deg is at the top and increases clockwise - MHKiT-Software#437 - Author: @jmcvey3 - Reviewer: @akeeste - Fixes: MHKiT-Software#427 ### Wave/Hindcast - Added a `hindcast_guard` exception-handling decorator (`hindcast_exceptions.py`) that surfaces a clear error on HSDS request failures, distinguishing the known NLR HSDS outage (MHKiT-Software#450) from other failures - MHKiT-Software#449 - Author: @simmsa - Reviewer: @akeeste ## Maintenance - Added Python 3.13 support - MHKiT-Software#445 - Author: @simmsa - Reviewer: @akeeste - Fixes: MHKiT-Software#441 - Added [pandas 3](https://pandas.pydata.org/community/blog/pandas-3.0.html) Support - Updated the pandas dependency to allow pandas 3.x - Added a compatibility shim for NDBC missing-value handling related to pandas 3 object to String dtype api changes - Fixed a deprecated period alias in tests - MHKiT-Software#443 - Author: @simmsa - Reviewer: @akeeste - Fixes: MHKiT-Software#440 - Updated GitHub Actions CI, expanded installation/developer documentation, refreshed dev environment, and trimmed dependencies - Refactored optional dependencies - Standardized conda/conda-forge environment builds - Scoped black linting to changed files - MHKiT-Software#436 - Author: @simmsa - Reviewer: @akeeste - Update `rex` dependency to target pypi package to [`NLR-rex[hsds]>=0.5.0`](https://pypi.org/project/NLR-rex/) - MHKiT-Software#449 - Author: @simmsa - Reviewer: @akeeste
There are three main changes in this pull request: 1. Added the option to determine the number of datapoints in each FFT when creating the sound pressure PSDs. This was currently hardcoded to the maximum FFT length, i.e. the total number of datapoints in each window, which might be more resolution and require more storage space than desired 2. Changed names of bin and windowing related attributes on the PSDs to make them easier to understand 3. Gain was improperly added - the sign has been corrected Minor changes are refactoring some of the argument type checks to clean them up.
# MHKiT-Python v1.1.0 ## Additions * Acoustics: Add millicdecade and WISPR instrument support * Added millidecade spectral conversion * Added a WISPR hydrophone reader * Added a voltage-based `export_audio` resampling option * Refactored band-averaging to avoid losing information at frequency-band boundaries * #447 * Author: @jmcvey3 * Reviewer: @simmsa * DOLfYN: Add [Nortek Aquadopp](https://www.nortekgroup.com/oceanography/aquadopp-series) ADCP support * Added DOLfYN support for reading Aquadopp instruments * Cleaned Nortek parsing code * Simplified handling of the non-cabled ADV orientation flag * #434 * Author: @jmcvey3 * Reviewer: @akeeste * Examples: Add ADCP waves example: `example/adcp_waves_example.ipynb` * Added an example notebook showing how to ingest and analyze wave measurements from a dual-profile Nortek Signature 250 deployment at PacWave. * #430 * Author: @jmcvey3 * Reviewer: @akeeste ## Improvements ### Acoustics - Improved flexibility and robustness of the Acoustics module - Added a configurable FFT length for sound pressure PSDs - Renamed bin/windowing attributes for clarity - Fixed an incorrectly signed gain correction - #433 - Author: @jmcvey3 - Reviewer: @akeeste ### DOLfYN - Refactored PSD calculations to use `scipy.signal.welch` - Replaced DOLfYN's custom Welch-like PSD implementation (built each segment from bin slices, could not overlap FFT segments) with `scipy.signal.welch` - Removed the bin-based segment structure and added a `step` argument to control overlap - Dropped float32 casts in some functions in favor of float64 - Removed `fft.py` and renamed `tools/misc.py` to `tools.py` - Breaking Changes: - NaNs are no longer tolerated in PSD calculations - DOLfYN defaults to 0% overlap to preserve existing array shapes - Acoustics defaults to 50% overlap - PSD output time dimension renamed to `time_psd` - #452 - Author: @jmcvey3 - Reviewer: @akeeste, @simmsa - Improved handling of "averaged" profiles - Fixed handling of Nortek Signature dual-profile ADCP data by defaulting to "_avg" velocity variables when untagged ones are absent. - #430 - Author: @jmcvey3 - Reviewer: @akeeste - Critical PSD bugfix - Fixed a bug where individual FFTs received a 50% overlap twice, which corrupted the first and last spectrum of a timeseries. - #430 - Author: @jmcvey3 - Reviewer: @akeeste ### Examples - Added histograms to ADCP example - #448 - Author: @browniea - Reviewer: @akeeste ### River/IO - Fixed Qhull interpolation and D3D coordinate-system errors - #448 - Author: @browniea - Reviewer: @akeeste - Fixes: #442, #444 - Delft3D module updates - Added new Delft3D coordinate names - Added a new grid-convergence-index calculation function - Added support for xarray/netCDF4 input in the D3D module - #428 - Author: @browniea - Reviewer: @akeeste ### Wave - NDBC Directional Wave Units - Fixed NDBC directional wave spectrum output to return degrees instead of radians - Updated polar plots so 0 deg is at the top and increases clockwise - #437 - Author: @jmcvey3 - Reviewer: @akeeste - Fixes: #427 ### Wave/Hindcast - Added a `hindcast_guard` exception-handling decorator (`hindcast_exceptions.py`) that surfaces a clear error on HSDS request failures, distinguishing the known NLR HSDS outage (#450) from other failures - #449 - Author: @simmsa - Reviewer: @akeeste ## Maintenance - Added Python 3.13 support - #445 - Author: @simmsa - Reviewer: @akeeste - Fixes: #441 - Added [pandas 3](https://pandas.pydata.org/community/blog/pandas-3.0.html) Support - Updated the pandas dependency to allow pandas 3.x - Added a compatibility shim for NDBC missing-value handling related to pandas 3 object to String dtype api changes - Fixed a deprecated period alias in tests - #443 - Author: @simmsa - Reviewer: @akeeste - Fixes: #440 - Updated GitHub Actions CI, expanded installation/developer documentation, refreshed dev environment, and trimmed dependencies - Refactored optional dependencies - Standardized conda/conda-forge environment builds - Scoped black linting to changed files - #436 - Author: @simmsa - Reviewer: @akeeste - Update `rex` dependency to target pypi package to [`NLR-rex[hsds]>=0.5.0`](https://pypi.org/project/NLR-rex/) - #449 - Author: @simmsa - Reviewer: @akeeste
# MHKiT-Python v1.1.0 ## Additions * Acoustics: Add millicdecade and WISPR instrument support * Added millidecade spectral conversion * Added a WISPR hydrophone reader * Added a voltage-based `export_audio` resampling option * Refactored band-averaging to avoid losing information at frequency-band boundaries * #447 * Author: @jmcvey3 * Reviewer: @simmsa * DOLfYN: Add [Nortek Aquadopp](https://www.nortekgroup.com/oceanography/aquadopp-series) ADCP support * Added DOLfYN support for reading Aquadopp instruments * Cleaned Nortek parsing code * Simplified handling of the non-cabled ADV orientation flag * #434 * Author: @jmcvey3 * Reviewer: @akeeste * Examples: Add ADCP waves example: `example/adcp_waves_example.ipynb` * Added an example notebook showing how to ingest and analyze wave measurements from a dual-profile Nortek Signature 250 deployment at PacWave. * #430 * Author: @jmcvey3 * Reviewer: @akeeste ## Improvements ### Acoustics - Improved flexibility and robustness of the Acoustics module - Added a configurable FFT length for sound pressure PSDs - Renamed bin/windowing attributes for clarity - Fixed an incorrectly signed gain correction - #433 - Author: @jmcvey3 - Reviewer: @akeeste ### DOLfYN - Refactored PSD calculations to use `scipy.signal.welch` - Replaced DOLfYN's custom Welch-like PSD implementation (built each segment from bin slices, could not overlap FFT segments) with `scipy.signal.welch` - Removed the bin-based segment structure and added a `step` argument to control overlap - Dropped float32 casts in some functions in favor of float64 - Removed `fft.py` and renamed `tools/misc.py` to `tools.py` - Breaking Changes: - NaNs are no longer tolerated in PSD calculations - DOLfYN defaults to 0% overlap to preserve existing array shapes - Acoustics defaults to 50% overlap - PSD output time dimension renamed to `time_psd` - #452 - Author: @jmcvey3 - Reviewer: @akeeste, @simmsa - Improved handling of "averaged" profiles - Fixed handling of Nortek Signature dual-profile ADCP data by defaulting to "_avg" velocity variables when untagged ones are absent. - #430 - Author: @jmcvey3 - Reviewer: @akeeste - Critical PSD bugfix - Fixed a bug where individual FFTs received a 50% overlap twice, which corrupted the first and last spectrum of a timeseries. - #430 - Author: @jmcvey3 - Reviewer: @akeeste ### Examples - Added histograms to ADCP example - #448 - Author: @browniea - Reviewer: @akeeste ### River/IO - Fixed Qhull interpolation and D3D coordinate-system errors - #448 - Author: @browniea - Reviewer: @akeeste - Fixes: #442, #444 - Delft3D module updates - Added new Delft3D coordinate names - Added a new grid-convergence-index calculation function - Added support for xarray/netCDF4 input in the D3D module - #428 - Author: @browniea - Reviewer: @akeeste ### Wave - NDBC Directional Wave Units - Fixed NDBC directional wave spectrum output to return degrees instead of radians - Updated polar plots so 0 deg is at the top and increases clockwise - #437 - Author: @jmcvey3 - Reviewer: @akeeste - Fixes: #427 ### Wave/Hindcast - Added a `hindcast_guard` exception-handling decorator (`hindcast_exceptions.py`) that surfaces a clear error on HSDS request failures, distinguishing the known NLR HSDS outage (#450) from other failures - #449 - Author: @simmsa - Reviewer: @akeeste ## Maintenance - Added Python 3.13 support - #445 - Author: @simmsa - Reviewer: @akeeste - Fixes: #441 - Added [pandas 3](https://pandas.pydata.org/community/blog/pandas-3.0.html) Support - Updated the pandas dependency to allow pandas 3.x - Added a compatibility shim for NDBC missing-value handling related to pandas 3 object to String dtype api changes - Fixed a deprecated period alias in tests - #443 - Author: @simmsa - Reviewer: @akeeste - Fixes: #440 - Updated GitHub Actions CI, expanded installation/developer documentation, refreshed dev environment, and trimmed dependencies - Refactored optional dependencies - Standardized conda/conda-forge environment builds - Scoped black linting to changed files - #436 - Author: @simmsa - Reviewer: @akeeste - Update `rex` dependency to target pypi package to [`NLR-rex[hsds]>=0.5.0`](https://pypi.org/project/NLR-rex/) - #449 - Author: @simmsa - Reviewer: @akeeste
There are three main changes in this pull request:
Minor changes are refactoring some of the argument type checks to clean them up.