121 the modified sinc smoother implementation by NusretSalli · Pull Request #213 · paucablop/chemotools

NusretSalli · 2025-10-23T19:07:17Z

This PR tries to close #121 - I don't think it is needed to be squashed before merging, but ultimately, this can be done if needed.

Description of the implementation

Based on the issue, it was suggested that a base FIR class was to be created, since Savitzky-Golay- and the Modified Sinc Smoother both belong to the same type of filter (which is Finite Impulse Response filter). The implementation has been carried out by creating/updating the following files:

chemotools/smooth/_Base.py has been updated to include the new class _BaseFIRFilter used for the Savtizky-Golay filter as well as the modified sinc smoother
chemotools/smooth/_modified_sinc_smoother.py has been created to implement the modified sinc smoother based on the following paper: Schmid, M.; Rath, D.; Diebold, U. "Why and How Savitzky–Golay Filters Should Be Replaced." ACS measurement science Au 2022, 2 (2), 185-196.
chemotools/smooth/_savitzky_golay_filter.py has been updated to have it use the _BaseFIRFilter class.
tests/smooth/test_modified_sinc.py have been added to test the functionality of the smoother.

Note: I realized that while the file was created to test Saviztky-Golay filter, it didn't have any. For now I haven't added any, but if needed, I can do that 😃

Results from the implementation

I've gone ahead and tried to test / highlight the changes (or for Savtizky-Golay's case; no changes 😆 ) to make sure the implementations can be used as intended. This has been done by using data from chemotools - namely the fermentation and coffee data.

To make sure that the functionality of Savtizky-Golay works as intended, the implementation from Scipy has been used as a "benchmark". From there I simply extract the kernels from both scipy's Savitzky-Golay filter and mine and compare them. I've also added the kernel for modified sinc smoother (see below):

Zooming in, we can see that the kernels are identical (this have been verified numerically as well but isn't as interesting as figures).

If there is anything, let me know! 😄

…-Smoother-Implementation

NusretSalli · 2025-10-23T19:31:55Z

Hmm, it seems like we have some issues regarding installing task where we hit an API rate limit 😢 - any experience with this?

It seems like the sphinx documentation didn't create any problems. I assume it is because it uses the taskfile.dev while the check that fails doesn't?

Any help would be much appreciated.

Edit: After re-running the workflow, the checks passed - don't know entirely why the mentioned error appeared. If any of you know why it might have happened, please let me know

Copilot

Pull Request Overview

This PR implements the Modified Sinc Filter smoother as suggested in issue #121 and refactors the existing Savitzky-Golay filter to use a common base class for FIR (Finite Impulse Response) filters.

Key changes:

Created a new _BaseFIRFilter base class to share common FIR filter functionality
Implemented the Modified Sinc Filter based on Schmid et al. (2022) with configurable parameters and passband-flattening corrections
Refactored SavitzkyGolayFilter to inherit from _BaseFIRFilter

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`chemotools/smooth/_base.py`	Added `_BaseFIRFilter` base class with common fit/transform logic and padding strategies
`chemotools/smooth/_modified_sinc_smoother.py`	New implementation of Modified Sinc Filter with kernel computation based on sinc core, Gaussian-like window, and optional kappa corrections
`chemotools/smooth/_savitzky_golay_filter.py`	Refactored to inherit from `_BaseFIRFilter` and implement kernel computation via scipy's savgol_coeffs
`tests/smooth/test_modified_sinc.py`	Comprehensive test suite for Modified Sinc Filter covering sklearn compliance, kernel properties, boundary modes, and axis behavior
`chemotools/smooth/__init__.py`	Added `ModifiedSincFilter` to public API exports

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-24T21:49:40Z

+
+class ModifiedSincFilter(_BaseFIRFilter):
+    """
+    Modified Sinc smoothing (MS) for denoising while preserving peak positions based on the paper "Why and How Savitzky–Golay Filters Should Be Replaced."


Corrected spelling of 'Savitzky–Golay' to 'Savitzky-Golay' for consistency with the filename and other references in the codebase.

Copilot · 2025-10-24T21:49:40Z

+
+    References
+    ----------
+    [1] Schmid, M.; Rath, D.; Diebold, U. "Why and How Savitzky–Golay Filters Should Be Replaced."


Corrected spelling of 'Savitzky–Golay' to 'Savitzky-Golay' for consistency with the filename and other references in the codebase.

Suggested change

[1] Schmid, M.; Rath, D.; Diebold, U. "Why and How Savitzky–Golay Filters Should Be Replaced."

[1] Schmid, M.; Rath, D.; Diebold, U. "Why and How Savitzky-Golay Filters Should Be Replaced."

paucablop

Hi @NusretSalli It looks awesome, very nice job, clear, and well structured!!! Thanks a lot!! 😄

paucablop · 2025-10-29T20:21:48Z

+        return np.moveaxis(out, -1, ax)
+
+    # --- hooks for subclasses ---
+    def _compute_kernel(self) -> np.ndarray:


Hi @NusretSalli the base class looks great, just a small comment, what do you think of using the decorator @abstractmethod for the class? a bit like this one (https://github.com/paucablop/chemotools/blob/82826e4d17c03d84a91e3650fb86662eadba3593/chemotools/baseline/_base.py#L114C1-L115C1)

Yes, this is something that I should have done. I'll add this right away! :)

paucablop · 2025-10-29T20:22:39Z

+        xp = self._pad_1d(x, m)
+        return np.convolve(xp, self.kernel_, mode="valid")  # same length as x
+
+    def _pad_1d(self, x: np.ndarray, m: int) -> np.ndarray:


I think this is really nice! 🤩

paucablop · 2025-11-02T17:52:44Z

+        if self.alpha <= 0:
+            raise ValueError("alpha must be positive.")
+
+        # ---- 1) Eq. 5: index -> normalized coordinate in [-1, 1] ----


Check (really nice!) - @paucablop review 2025/11/02😎

paucablop · 2025-11-02T18:13:35Z

+            dtype=np.float64,
+        )
+        # solve the system using linear algebra.
+        rhs = np.array([1.0, 0.0, 0.0], dtype=np.float64)


Clever calculation of A 🧠

paucablop · 2025-11-02T18:38:11Z

@@ -0,0 +1,200 @@
+# Authors: Nusret Emirhan Salli <nusret.emirhan.salli@gmail.com>


On top of the file, could you add this?

"""
The :mod:chemotools.smooth._modified_sinc_smoother module implements the Modified Sinc Filter (SGF) transformation.
"""

I will do that! I totally forgot this.

paucablop · 2025-11-02T18:40:05Z

Very nice Nusret!! 💪 I checked with the reference and it is great and easy to follow!! Thanks a lot for the clear and good implementation!

MothNik · 2025-11-07T16:48:49Z

+                ),
+                dtype=np.float64,
+            )
+        except Exception:


I would like to drop the note that this computation can lead to numerical problems, especially for higher polynomial degrees and window sizes (found to be useful here).

The same problem applies to scipy (see this issue).
For fixing this problem, the algorithm needs to be adapted (see here).

Just a side note, no need for immediate action 😅

NusretSalli added 13 commits September 25, 2025 23:58

Initial modified sinc smoother implemented

a4789a5

Base FIRfilter class added

87e8f27

Savitzky_golay_filter updated with FIRfilter

83c1e9e

modified sinc smoother implemented using the FIR class

55d7e6a

ran linting and formatting

731bee2

Added authorship to files

f5606ed

Minor fixes to implementation of modified sinc smoother

b8d8f8e

fixed and changed the comments to add more explanation to the approach

e00bf47

Linting performed

e827156

Merge remote-tracking branch 'origin/main' into 121-The-Modified-Sinc…

39d10e2

…-Smoother-Implementation

Small corrections made before creating PR

2cab642

added test for kappa corrections

e1d829f

Fixed type return from "Self" to "_BaseFIRFilter"

ccb76c6

NusretSalli requested a review from paucablop October 23, 2025 19:30

NusretSalli self-assigned this Oct 24, 2025

NusretSalli requested a review from Copilot October 24, 2025 21:49

Copilot AI reviewed Oct 24, 2025

View reviewed changes

paucablop previously approved these changes Nov 2, 2025

View reviewed changes

Added changes suggested during PR review

0a6010a

NusretSalli dismissed paucablop’s stale review via 0a6010a November 2, 2025 20:29

paucablop merged commit f6909f7 into paucablop:main Nov 2, 2025
5 checks passed

MothNik reviewed Nov 7, 2025

View reviewed changes

NusretSalli deleted the 121-The-Modified-Sinc-Smoother-Implementation branch November 24, 2025 19:16

	[1] Schmid, M.; Rath, D.; Diebold, U. "Why and How Savitzky–Golay Filters Should Be Replaced."
	[1] Schmid, M.; Rath, D.; Diebold, U. "Why and How Savitzky-Golay Filters Should Be Replaced."

		@@ -0,0 +1,200 @@
		# Authors: Nusret Emirhan Salli <nusret.emirhan.salli@gmail.com>

Conversation

NusretSalli commented Oct 23, 2025

Description of the implementation

Results from the implementation

Uh oh!

NusretSalli commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

paucablop left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NusretSalli commented Oct 23, 2025 •

edited

Loading

paucablop left a comment •

edited

Loading