Skip to content

Conversation

@cVogl97
Copy link
Contributor

@cVogl97 cVogl97 commented Dec 1, 2025

This PR adds a few processors which are, in principle, available through numpy, but which are not easy to use in dspeed out of the box (at least for me). The behavior of time_point_thresh is documented more precicley and a new, similar, function, time_point_thresh_nopol, is added which does not require positive polarity in the waveform to report a threshold crossing.

Specifically, this PR

  • adds the sum processor
  • adds the mean processor
  • adds the mean_below_threshold processor
  • adds the sort processor (wraps numpy.sort)
  • adds tests the for sum, mean, mean_below_threshold and sort processors
  • adds time_point_thresh_nopol processor
    • does not impose condition on polarity
  • updated docstring of time_point_thresh
    • to clearly state that positive polarity when going forward in time is required for a threshold crossing to be found
  • adds tests for time_point_thresh_nopol and tests which differentiate from time_point_thresh.

Copilot AI and others added 17 commits November 27, 2025 16:28
updated docstring of time_point_thresh and added time_point_thresh_nopol
Add arithmetic processors: sum and mean
@codecov
Copy link

codecov bot commented Dec 1, 2025

Codecov Report

❌ Patch coverage is 89.02439% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.06%. Comparing base (4f5f739) to head (973e366).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/dspeed/processors/arithmetic.py 88.46% 6 Missing ⚠️
src/dspeed/processors/time_point_thresh.py 85.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
+ Coverage   62.52%   63.06%   +0.54%     
==========================================
  Files          58       60       +2     
  Lines        3920     4002      +82     
==========================================
+ Hits         2451     2524      +73     
- Misses       1469     1478       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cVogl97 cVogl97 marked this pull request as ready for review December 1, 2025 10:58
@cVogl97 cVogl97 marked this pull request as draft December 2, 2025 10:11
@ggmarshall
Copy link
Contributor

Maybe a quick comment, I'm not really a fan of adding code that could be run in another way, at the end of the day we have to maintain everything in this repo and therefore keeping it as minimal as possible is beneficial. I would rather update tutorials and docs on how to run these

@cVogl97
Copy link
Contributor Author

cVogl97 commented Dec 8, 2025

In principle I agree. I have, unfortunately, not been sucessfull with getting nump.sum or numpy.mean to work in a dspeed config file. If you, or perhaps @iguinn, have done so, I'd be happy to read an example.

numpy.sort is a different story since it does not offer an output argument. This one cannot be used without a new processor, as far as I understand dspeed. I am also not sure how to do mean_below_threshold without adding a new processor.

@ggmarshall
Copy link
Contributor

'''
wf_sum:
function: sum
module: numpy
args:
- wf_window
- 1
- '''f'''
- wf_sum
kwargs:
signature: (n),(),()->()
types: fic->f
'''

@iguinn
Copy link
Collaborator

iguinn commented Dec 8, 2025

numpy.sort is a different story since it does not offer an output argument. This one cannot be used without a new processor, as far as I understand dspeed. I am also not sure how to do mean_below_threshold without adding a new processor.

Yeah that is correct, as things are right now. There actually is a wrapper class for external functions that I could have the kwargs fields help with, that can handle these cases.

In terms of philosophy on these things I agree with George, at least as far as dspeed goes; dspeed can actually work with arbitrary modules though, so writing an extension package that you import processors from is an intended workflow, and in that case you can choose your own philosophy. I actually want to move some of our processors into a legend-specific repository, but it's not at all high on my priority list...

@iguinn
Copy link
Collaborator

iguinn commented Dec 21, 2025

So, thinking more about this one, there actually is a case for implementing some of the reductions over a range of values (which was actually how this was originally written). One important case for this would be for variable length values, where you may want to get the mean as mean(val, 0, len(val), val_mean). To do this as is, you would first have to call the windower before calling mean. And of course, this could always be used in place of np.mean(val, axis=-1).

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.

3 participants