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 1 commit
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
13 changes: 13 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,9 @@ 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.
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.

This adds a Sphinx .. deprecated:: directive in the parameter docstring without a version argument. If Sphinx processes this docstring as reST, the directive usually requires .. deprecated:: <version> and can otherwise emit a parsing warning/error. Please provide the version or convert this to plain text so doc builds remain clean.

Suggested change
.. deprecated::
Use of ``kde_smoothing=True`` is deprecated and will be removed
in a future version.
Deprecated: use of ``kde_smoothing=True`` is deprecated and will be
removed in a future version.

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

@tomaskontrimas tomaskontrimas Apr 25, 2026

Choose a reason for hiding this comment

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

fixed

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 +157,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',
DeprecationWarning,
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 +384,9 @@ 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.
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.

This second docstring also uses .. deprecated:: without a version argument, which can trigger Sphinx parsing warnings/errors if interpreted as reST. Please add a version (.. deprecated:: <version>) or rewrite as plain text to avoid doc build issues.

Suggested change
.. deprecated::
Use of ``kde_smoothing=True`` is deprecated and will be removed
in a future version.
Deprecated: Use of ``kde_smoothing=True`` is deprecated and will
be removed in a future version.

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

@tomaskontrimas tomaskontrimas Apr 25, 2026

Choose a reason for hiding this comment

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

fixed

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
3 changes: 3 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,9 @@ 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.
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 docstring introduces a Sphinx .. deprecated:: directive without a version argument. Since Sphinx generally requires a version for this directive, this may cause reST parsing warnings/errors during documentation builds. Consider adding the deprecation version or using a non-directive deprecation note.

Suggested change
.. deprecated::
Use of ``kde_smoothing=True`` is deprecated and will be removed
in a future version.
Deprecated: Use of ``kde_smoothing=True`` is deprecated and will be
removed in a future version.

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

@tomaskontrimas tomaskontrimas Apr 25, 2026

Choose a reason for hiding this comment

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

fixed

Apply a KDE-based smoothing to the data-driven background pdf.
Default: False.
minimizer_impl : str | "LBFGS"
Expand Down
3 changes: 3 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,9 @@ 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.
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 docstring adds a Sphinx .. deprecated:: directive without a version argument. Sphinx typically expects .. deprecated:: <version>; omitting it can produce a doc build warning/error. Please add the appropriate version string or rephrase this as plain text to avoid directive parsing issues.

Suggested change
.. deprecated::
Use of ``kde_smoothing=True`` is deprecated and will be removed
in a future version.
Deprecated: Use of ``kde_smoothing=True`` is deprecated and will be
removed in a future version.

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

@tomaskontrimas tomaskontrimas Apr 25, 2026

Choose a reason for hiding this comment

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

fixed

Apply a KDE-based smoothing to the data-driven background pdf.
Default: False.
minimizer_impl : str
Expand Down
3 changes: 3 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,9 @@ 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.
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 new docstring uses the Sphinx .. deprecated:: directive without a version argument. In Sphinx this directive normally requires a version (e.g. .. deprecated:: 1.2), otherwise it can emit a reST parsing warning/error during doc builds. Consider either supplying the correct deprecation version or replacing this directive with plain text (e.g. a short "Deprecated" note) that won’t trigger directive parsing.

Suggested change
.. deprecated::
Use of ``kde_smoothing=True`` is deprecated and will be removed
in a future version.
Deprecated: use of ``kde_smoothing=True`` is deprecated and will be
removed in a future version.

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

@tomaskontrimas tomaskontrimas Apr 25, 2026

Choose a reason for hiding this comment

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

fixed

Apply a KDE-based smoothing to the data-driven background pdf.
Default: False.
minimizer_impl : str
Expand Down