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

reduce pvss computation via find_peaks #195

Merged
merged 12 commits into from
Jun 3, 2022
Merged

Conversation

S-Hanly
Copy link
Contributor

@S-Hanly S-Hanly commented May 18, 2022

I'll need to dig into the batch tests, I think it's because I added the resultant...

But basically, I'm using find_peaks to only process the shock response function on peak events. For the recording I was testing on, it takes over 2 minutes to calculate the spectrum on the whole range, but then 2 seconds by just using the peaks. For shorter recordings with repeating events it's not as drastic a savings but for one went from 7 seconds to 2 seconds. The question is... by doing this we may be missing some lower frequency content slightly.

Here's one axis comparing the full vs the peak calculation and they match exactly (there are two lines drawn over each other):
image

But for the less prominent axes we can see this deviation slightly. I'm not sure if this a big enough deal where we will allow the user to effectively turn off this performance feature via setting max_time to a very large number.
image

@S-Hanly S-Hanly requested a review from StokesMIDE May 18, 2022 21:15
@S-Hanly
Copy link
Contributor Author

S-Hanly commented May 18, 2022

@CrepeGoat @theflanman you may have a comment, no pressure though (hope all is well)!

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2022

Codecov Report

Merging #195 (5421164) into development (3be10ed) will increase coverage by 0.46%.
The diff coverage is 93.82%.

@@               Coverage Diff               @@
##           development     #195      +/-   ##
===============================================
+ Coverage        69.09%   69.56%   +0.46%     
===============================================
  Files               34       34              
  Lines             2964     3036      +72     
===============================================
+ Hits              2048     2112      +64     
- Misses             916      924       +8     
Impacted Files Coverage Δ
endaq/batch/analyzer.py 91.83% <ø> (ø)
endaq/calc/stats.py 92.43% <ø> (-0.85%) ⬇️
endaq/plot/plots.py 63.56% <93.44%> (+7.90%) ⬆️
endaq/calc/shock.py 88.63% <95.00%> (-0.19%) ⬇️
endaq/calc/utils.py 85.00% <0.00%> (-1.67%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@S-Hanly
Copy link
Contributor Author

S-Hanly commented May 19, 2022

@S-Hanly
Copy link
Contributor Author

S-Hanly commented May 19, 2022

Next I will add a function to generate these 4 coordinate plots:

@S-Hanly S-Hanly mentioned this pull request Jun 1, 2022
@S-Hanly S-Hanly mentioned this pull request Jun 1, 2022
@S-Hanly
Copy link
Contributor Author

S-Hanly commented Jun 1, 2022

@StokesMIDE in looking back at older commits, the bulk of this PR was passing on my second commit: https://github.com/MideTechnology/endaq-python/pull/195/checks?sha=d1968621e33c02ebf18046e7d024bdb2d3dc0724. So I went ahead and made a new branch off that commit to try and do a PR on (#197) but that's now failing... no code has changed since it passed.

So in this updated PR that is now passing, I just commented out the unit tests that were causing issues, they pass when I run it locally. I'm not sure how to proceed here? Do we merge this in and then create an issue to later figure out how to get these unrelated unit tests to pass again someday?

Copy link
Member

@StokesMIDE StokesMIDE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tripartate plot is out of my wheelhouse, so I can't critique too deeply, but the code looks sound.

@S-Hanly S-Hanly merged commit b4f56b1 into development Jun 3, 2022
@S-Hanly S-Hanly deleted the feature/pvss-from-peaks branch June 10, 2022 00:32
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.

None yet

3 participants