-
Notifications
You must be signed in to change notification settings - Fork 13
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
XMas-Day-4: DataSet<T>: helpers, estimators, math functions, restructure DataSet #494
Conversation
e609fb4
to
ff23424
Compare
…Set, and add tests - Introduced helper utilities for DataSet<T> access and manipulation. - Implemented simple estimators and basic mathematical functions. - Restructured DataSet<T>: - Added explicit Range<T>::min and Range<T>::max. - Removed `signal_error` in favor of `UncertainValue<T>`. - Enhanced UncertainValue<T>: - Added `isfinite` and `log10` functions. - Implemented the `<<` operator for easy formatting. - Added unit tests for DataSetEstimators, DataSetMath, DataSetTestFunctions, and DataSet functionalities. - Updated accessors and utility functions to support new features. Signed-off-by: Ralph J. Steinhagen <[email protected]>
ff23424
to
0f7e5a1
Compare
Added for (const auto& zeta : {0.01, 0.1, 0.99}) {
auto filter = Filter<double>(iir::designResonatorPhysical(fs, 10., zeta));
std::vector<double> yValue(xValues.size());
std::vector<double> yFiltered(xValues.size());
std::transform(xValues.cbegin(), xValues.cend(), yValue.begin(), [](double t) { return t < 0.05 ? 0.0 : 1.0; });
std::transform(yValue.cbegin(), yValue.cend(), yFiltered.begin(), [&filter](double y) { return filter.processOne(y); });
expect(nothrow([&] { chart.draw(xValues, yFiltered, fmt::format("resonance@zeta={}", zeta)); })) << fmt::format("resonance@zeta={} does not throw", zeta);
}
expect(nothrow([&] { chart.draw(); })); |
* added 2nd-order resonance filter * added apply[Symmetric]Filter(..) math function * added enum to declare if math operation is performed `ProcessMode::InPlace` or on a `ProcessMode::Copy` * added enum to declare if estimates are added as meta-info (`ProcessMode::None`) * improved recurring `gr::dataset::detail::checkIndexRange(..)` helper function * fixed some missing `(ds, minIndex, maxIndex, ..)` method signatures. * added `gr::cast<T>(..)` to suppress float-double compiler warnings in templating code that cannot be resolved programmatically * added `filter::applyMedian(..)` * added `filter::applyRms(..)` * added `filter::applyPeakToPeak(..)` Signed-off-by: Ralph J. Steinhagen <[email protected]>
83bcce7
to
0e18e1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR!
The code in this PR looks very good and is thoroughly tested.
There are a few suggestions regarding consistency and optimizations that I believe should be reviewed before merge.
Once those points are resolved, this PR will be ready for merging.
algorithm/include/gnuradio-4.0/algorithm/dataset/DataSetEstimators.hpp
Outdated
Show resolved
Hide resolved
algorithm/include/gnuradio-4.0/algorithm/dataset/DataSetEstimators.hpp
Outdated
Show resolved
Hide resolved
algorithm/include/gnuradio-4.0/algorithm/dataset/DataSetEstimators.hpp
Outdated
Show resolved
Hide resolved
T minVal = getMinimum(dataSet, indexMin, indexMax, signalIndex).value().value; | ||
T maxVal = getMaximum(dataSet, indexMin, indexMax, signalIndex).value().value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be checked for std::nullopt
and return std::numeric_limits<TValue>::quiet_NaN()
, otherwise an exception can be thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is guaranteed to always have a value in this context of the earlier checkIndexRange(..)
check above and also the same check within the get[Minimum, Maximum](...)
functions.
algorithm/include/gnuradio-4.0/algorithm/dataset/DataSetEstimators.hpp
Outdated
Show resolved
Hide resolved
} | ||
|
||
template<std::ranges::random_access_range T, typename TValue = typename T::value_type> | ||
[[nodiscard]] constexpr TValue computeInterpolatedFWHM(const T& data, std::size_t index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as for computeFWHM
. Should be clarified in the documentation that function returns a difference in indices but not actual values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be clear from the context here since there is no x-axis provided. Thus any width is always expressed in terms of the indexing.
This is different for the FWHM estimators that work on DataSet<T>
though.
T sum = 0; | ||
T count = 0; | ||
for (const auto& val : finiteValues) { | ||
sum += val; | ||
count += T(1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only here but a general question, should we stick to use more std::ranges
?
T sum = 0; | |
T count = 0; | |
for (const auto& val : finiteValues) { | |
sum += val; | |
count += T(1); | |
} | |
T sum = std::accumulate(finiteValues.begin(), finiteValues.end(), T(0)); | |
T count = static_cast<T>(std::ranges::distance(finiteValues)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't use it here because this performs two accumulates/counting. To note this is because the finiteValues
and signalRange
views can have different sizes depending on how many actual 'NaN' are present in the data range.
We should discuss from a design point-of-view, whether we support or intercept 'NaN' floating point values. I took this implementation/choice based upon one of our other DataSet
implementing math libraries (in Chart-FX) that supports 'NaN' by default. However, this choice is opinionated and we may equally disallow or explicitly not support this for GR4.
Signed-off-by: rstein <[email protected]>
21f8bf2
to
4ecda2f
Compare
Quality Gate failedFailed conditions |
On the fourth day of Christmas 🎄 👼 4️⃣ 🎄 , my code base gave to me...
Helpers, estimators, and math functions, oh my!
This PR enhances the
DataSet<T>
by introducing:Helper Methods:
std::span<T>
andstd::span<const T>
depending on constness.DataSet Restructure:
Range<T>
with explicitmin
andmax
members for clear range definitions.signal_error
in favor of usingUncertainValue<T>
to represent uncertainties.UncertainValue Enhancements:
isfinite
andlog10
functions for better mathematical operations.<<
operator to facilitate easy formatting and output.Estimators and Math Functions:
DataSet<T>
.Unit Tests:
Utility Updates:
Documentation:
Feel free to explore the new additions and share your feedback!
Happy coding and joyful holidays! 🎅🤶🎁
Code Example:
P.S. Did I mention that I like unit-tests?