Want some suggestions on the implementation of the mean filter, and sincerely expect me some advice

// Mean Filter with Fixed Len,suitable for post-processing
pub struct FixLenMeanFilterPP<T> {
    data: RefCell<VecDeque<T>>,
    len: RefCell<usize>,
    sum: RefCell<T>,
    average: RefCell<T>,
}

impl<T> FixLenMeanFilterPP<T>
where
    T: std::ops::Add<T, Output = T>,
    T: std::ops::Sub<T, Output = T>,
    T: std::ops::Div<T, Output = T>,
    T: for<'a> std::iter::Sum<&'a T>,
    T: Copy + From<usize>,
{
    pub fn new(vec: Vec<T>) -> Self {
        let l = vec.len();
        let s = vec.iter().sum();
        Self {
            len: RefCell::new(l),
            sum: RefCell::new(s),
            average: RefCell::new(s / l.into()),
            data: RefCell::new(vec.into_iter().collect()),
        }
    }

    pub fn add(&self, val: T) -> &Self {
        let mut vecdeque = self.data.borrow_mut();
        let x = vecdeque.pop_front().expect("The Mean filter is empty!");
        vecdeque.push_back(val);
        self.update_average(val, x);
        &self
    }

    pub fn average(&self) -> T {
        *self.average.borrow()
    }

    fn update_average(&self, val_in: T, val_out: T) {
        let mut val = self.sum.borrow_mut();
        *val + val_in - val_out;
        self.average
            .replace(*self.sum.borrow() / (*self.len.borrow()).into());
    }
}

// Mean Filter with Fixed Len,suitable for real-time processing
pub struct FixLenMeanFilterRTP<T> {
    data: RefCell<VecDeque<T>>,
    curlen: RefCell<usize>,
    len: RefCell<usize>,
    sum: RefCell<T>,
    average: RefCell<T>,
}

impl<T> FixLenMeanFilterRTP<T>
where
    T: std::ops::Add<T, Output = T>,
    T: std::ops::Sub<T, Output = T>,
    T: std::ops::Div<T, Output = T>,
    T: for<'a> std::iter::Sum<&'a T>,
    T: Copy + From<usize> + Default,
{
    pub fn new(len: usize) -> Self {
        Self {
            curlen: RefCell::new(0),
            len: RefCell::new(len),
            sum: RefCell::new(T::default()),
            average: RefCell::new(T::default()),
            data: RefCell::new(VecDeque::new()),
        }
    }

    fn is_full(&self) -> bool {
        *self.curlen.borrow() == *self.len.borrow()
    }

    pub fn add(&self, val: T) -> &Self {
        if self.is_full() {
            let mut data = self.data.borrow_mut();
            let x = data.pop_front().unwrap();
            data.push_back(val);
            self.update_average(val, x);
        } else {
            let mut data = self.data.borrow_mut();
            data.push_back(val);
            self.sum.replace_with(|sum| *sum + val);
            self.curlen.replace_with(|len| *len + 1);
        }
        &self
    }

    pub fn average(&self) -> T {
        *self.average.borrow()
    }

    fn update_average(&self, val_in: T, val_out: T) {
        let mut val = self.sum.borrow_mut();
        *val + val_in - val_out;
        self.average
            .replace(*self.sum.borrow() / (*self.len.borrow()).into());
    }
}

Why is every field a RefCell? Unless you have a specific reason, I'd recommend against it and change the mutating methods to take &mut self. It gives you more compile-time safety.

Using RefCells will probably hurt performance too. You are performing the associated runtime check every single time you are updating the filter state with a new sample.

In other words, if not necessary, use RefCell as little as possible

BTW, your code is way too complicated and non-idiomatic even without RefCells:

  • The trait bounds for expressing numerical operations over floating-point numbers is num_traits::Float
  • You call the mean an average in the struct definition (and as a method), but the struct itself is called a "mean" filter. Don't use multiple terms for the same thing – it's confusing. If you call the type a "mean filter", then call the mean "the mean", not "the average".
  • It is wrong to initialize the mean to the default value (which is zero) in FixLenMeanFilterRTP::new(). The mean of an empty set is undefined (NaN), not zero.
  • FixLenMeanFilterRTP::add() is wrong. You never update the mean unless self.is_full().
  • You are forcing an allocation on the caller by taking a Vec<T> instead of an iterator.
  • The len field in FixLenMeanFilterPP is unused.
  • Similarly, the curlen field in FixLenMeanFilterRTP is redundant (it's just self.data.len()).
  • The statement *val + val_in - val_out; doesn't do anything. (Also, you've got the signs backwards.)
  • Even if you needed interior mutability, RefCell for Copy types such as usize is useless (unnecessarily expensive). You could have just used plain Cell if it were necessary (here it isn't).

All in all, here's a correct and simpler implementation.

Very good suggestions, thanks you very much!

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.