Skip to content

Deprecate kde_smoothing=True argument #310

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.

Sign up for GitHub

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

Open
tomaskontrimas wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Deprecate kde_smoothing=True argument #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a68ad2
Deprecate `kde_smoothing=True` argument
tomaskontrimas Apr 25, 2026
95d7c54
Fix sphinx directive based on copilot suggestion. We don't know the e…
tomaskontrimas Apr 25, 2026
51a7ea1
Use FutureWarning
tomaskontrimas Apr 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions skyllh/analyses/i3/publicdata_ps/backgroundpdf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import warnings

import numpy as np
from scipy.stats import (
gaussian_kde,
Expand Down Expand Up @@ -83,6 +85,8 @@ def __init__(
The smoothing filter to use for smoothing the energy histogram.
If None, no smoothing will be applied.
kde_smoothing : bool
Deprecated: use of ``kde_smoothing=True`` is deprecated and will be
removed in a future version.
Apply a kde smoothing to the energy pdf for each bin in sin(dec).
This is useful for signal injections, because it ensures that the
background is not zero when injecting high energy events.
Expand Down Expand Up @@ -152,6 +156,11 @@ def __init__(
self._hist_mask_mc_covered_zero_physics = h > 0

if kde_smoothing:
warnings.warn(
'The kde_smoothing option is deprecated and will be removed in a future version',
FutureWarning,
stacklevel=2,
)
Comment on lines +159 to +163
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

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

The issue/PR description asks for a logging message when kde_smoothing=True is used, but the implementation uses warnings.warn(..., DeprecationWarning). In addition, DeprecationWarning is ignored by default for library code, so most users won’t see it. Consider switching to the project’s logging (e.g. a module logger and logger.warning(...)) and/or using a warning class that is shown by default (commonly FutureWarning) so the deprecation notice reliably reaches users.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@tomaskontrimas tomaskontrimas Apr 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

fixed with FutureWarning as we want users to see it

# If a bandwidth is passed, apply a KDE-based smoothing with the
# given bandwidth parameter as bandwidth for the fit.
kde_pdf_list = []
Expand Down Expand Up @@ -374,6 +383,8 @@ def __init__(self, data_exp, logE_binning, sinDec_binning, smoothing_filter=None
The smoothing filter to use for smoothing the energy histogram.
If None, no smoothing will be applied.
kde_smoothing : bool
Deprecated: use of ``kde_smoothing=True`` is deprecated and will be
removed in a future version.
Apply a kde smoothing to the energy pdf for each bin in sin(dec).
This is useful for signal injections, because it ensures that the
background is not zero when injecting high energy events.
Expand Down
2 changes: 2 additions & 0 deletions skyllh/analyses/i3/publicdata_ps/time_dependent_ps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,8 @@ def create_analysis(
gamma_max : float
Upper bound for gamma fit.
kde_smoothing : bool
Deprecated: use of ``kde_smoothing=True`` is deprecated and will be
removed in a future version.
Apply a KDE-based smoothing to the data-driven background pdf.
Default: False.
minimizer_impl : str | "LBFGS"
Expand Down
2 changes: 2 additions & 0 deletions skyllh/analyses/i3/publicdata_ps/time_integrated_ps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,8 @@ def create_analysis(
gamma_max : float
Upper bound for gamma fit.
kde_smoothing : bool
Deprecated: use of ``kde_smoothing=True`` is deprecated and will be
removed in a future version.
Apply a KDE-based smoothing to the data-driven background pdf.
Default: False.
minimizer_impl : str
Expand Down
2 changes: 2 additions & 0 deletions skyllh/analyses/i3/publicdata_ps/time_integrated_ps_function_energy_spectrum.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ def create_analysis(
e_peak_max : float
Upper bound for energy peak fit,
kde_smoothing : bool
Deprecated: use of ``kde_smoothing=True`` is deprecated and will be
removed in a future version.
Apply a KDE-based smoothing to the data-driven background pdf.
Default: False.
minimizer_impl : str
Expand Down