Deprecate kde_smoothing=True argument#310
Conversation
There was a problem hiding this comment.
Pull request overview
This PR deprecates the kde_smoothing=True option used for KDE-based smoothing of the data-driven background energy PDF in the IceCube public-data point-source analyses (issue #295).
Changes:
- Adds deprecation notes to
kde_smoothingparameter documentation across multiplecreate_analysis(...)entry points. - Emits a runtime deprecation notice when
kde_smoothing=Trueis used in the background PDF implementation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
skyllh/analyses/i3/publicdata_ps/time_integrated_ps_function_energy_spectrum.py |
Adds deprecation wording to the kde_smoothing parameter docstring. |
skyllh/analyses/i3/publicdata_ps/time_integrated_ps.py |
Adds deprecation wording to the kde_smoothing parameter docstring. |
skyllh/analyses/i3/publicdata_ps/time_dependent_ps.py |
Adds deprecation wording to the kde_smoothing parameter docstring. |
skyllh/analyses/i3/publicdata_ps/backgroundpdf.py |
Adds deprecation wording in docstrings and a runtime deprecation notice when kde_smoothing=True. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .. deprecated:: | ||
| Use of ``kde_smoothing=True`` is deprecated and will be removed | ||
| in a future version. |
There was a problem hiding this comment.
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.
| .. 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. |
| .. deprecated:: | ||
| Use of ``kde_smoothing=True`` is deprecated and will be removed | ||
| in a future version. |
There was a problem hiding this comment.
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.
| .. 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. |
| .. deprecated:: | ||
| Use of ``kde_smoothing=True`` is deprecated and will be removed | ||
| in a future version. |
There was a problem hiding this comment.
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.
| .. 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. |
| .. deprecated:: | ||
| Use of ``kde_smoothing=True`` is deprecated and will be removed | ||
| in a future version. |
There was a problem hiding this comment.
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.
| .. 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. |
| .. deprecated:: | ||
| Use of ``kde_smoothing=True`` is deprecated and will be removed | ||
| in a future version. |
There was a problem hiding this comment.
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.
| .. 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. |
| warnings.warn( | ||
| 'The kde_smoothing option is deprecated and will be removed in a future version', | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
fixed with FutureWarning as we want users to see it
Fixes #295