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

Feature: Impulsive metrics (kurtosis and energy windowed SPL RMS) #143

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

caplinje-NOAA
Copy link

Hello,

Finally ready to submit this, the changes can be summarized as:

  • added kurtosis (in utils, signal, acoustic_file, and units) Add kurtosis #106
  • added energy windowing method to utils
  • added option for energy window computation of rms
  • added implusive flag to _event.analyze which calculates all impulsive related metrics
  • made a small modification to path handling for testing (was necessary in my IDE to run tests)
  • made some improvements to docstrings

In terms of new math, the primary things are the kurtosis and energy window calculations. For kurtosis, I explored the best implementation and documented it here. For the energy window calculation, I followed the standard method from the most often cited reference. In both cases I added tests which compare against known/expected results for each. Additionally, I built a comprehensive test of all of the new metrics using some example pile driving acoustic data and previously analyzed results.

In terms of testing, everything is passing with the exception of TestPlots.test_summary_plot and TestDataset.test_generate_dataset.

Here are the two exceptions from the failed tests:

tests\test_dataset.py:66 (TestDataset.test_generate_dataset)
self = <tests.test_dataset.TestDataset testMethod=test_generate_dataset>

>           raise FileNotFoundError('The path %s does not exist. Please choose another one.' % folder_path)
E           FileNotFoundError: The path tests/test_data does not exist. Please choose another one.

..\pypam\acoustic_survey.py:511: FileNotFoundError


================ 2 failed, 50 deselected, 3 warnings in 14.07s ================
FAILED                       [100%]
tests\test_plots.py:78 (TestPlots.test_summary_plot)
self = <tests.test_plots.TestPlots testMethod=test_summary_plot>

        if location is not None:
            if pvlib is None:
>               raise Exception('To use this feature it is necessary to install pvlib ')
E               Exception: To use this feature it is necessary to install pvlib

..\pypam\plots.py:382: Exception

Please let me know if you would like me to split any of this out and submit separate pull requests, as I am realizing this might be a lot for a single pull request. For example, if you are only interested in including kurtosis, I would definitely understand, as many of my edits specifically relate to performing analysis of anthro impulsive sources.

Again, it was been really fun to work with and learn this package, thanks!

caplinje-NOAA and others added 16 commits November 10, 2024 15:50
* updated tests to include relative paths to test data, allowing them to run anywhere

* corrected issue related to non-zero bin overlaps
* updated tests to include relative paths to test data, allowing them to run anywhere

* draft metrics commit

* add impulsive metrics and associated tests

* small corrections/refactor

* added kurtosis units, added test for using apply multiple for kurtosis, updated gitignore

* updated docstrings

* clean up test_impulsive_metrics.py
fix invalid escape
added numpy import
removed unnecessary import
@caplinje-NOAA
Copy link
Author

Small update here: after adding the optional dependencies, all tests pass.

@caplinje-NOAA
Copy link
Author

caplinje-NOAA commented Nov 18, 2024

Apologies for the messy commit history, and I should note that this branch also contains the bin overlap fix related to #127. Again, if you want me to more clearly separate out all of these changes, please let me know and I can submit pull requests from simpler/cleaner branches.

@cparcerisas
Copy link
Collaborator

Hi, no worries! Thank you so much for all the nice work, it looks great! I am now busy with some other work but I will definitely check it out in in two weeks :) Sorry for the delay.

@caplinje-NOAA
Copy link
Author

Absolutely no rush on my end - happy to work through this whenever it makes sense. Thanks!

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.

2 participants