Conversation
Contributor
Reviewer's GuideThis PR raises the minimum Python requirement to 3.11 (dropping 3.10), updates CI and linting configurations accordingly, applies list-comprehension workarounds for astropy Time conversions to prevent segfaults, standardizes docstring note block formatting across multiple modules, adjusts a PSF comparison test to use default tolerance, and adds an initial changelog entry for upcoming breaking changes. Class diagram for Time conversion workaround in calibration utilitiesclassDiagram
class Table {
T_START
T_STOP
WAVELNTH
EFF_WVLN
EFF_AREA
}
class Time {
+scale: str
}
class get_correction_table {
+table: Table
+new_start_times: list~Time~
+new_end_times: list~Time~
}
class get_pointing_table {
+table: Table
+new_start_times: list~Time~
+new_end_times: list~Time~
}
get_correction_table --> Table
get_correction_table --> Time
get_pointing_table --> Table
get_pointing_table --> Time
Class diagram for docstring note formatting changesclassDiagram
class respike {
+docstring: str
}
class calculate_psf {
+docstring: str
}
class deconvolve {
+docstring: str
}
class degradation {
+docstring: str
}
class eve_correction {
+docstring: str
}
class estimate_error {
+docstring: str
}
respike : docstring notes now consistently indented
calculate_psf : docstring notes now consistently indented
deconvolve : docstring notes now consistently indented
degradation : docstring notes now consistently indented
eve_correction : docstring notes now consistently indented
estimate_error : docstring notes now consistently indented
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The astropy Time workaround directly assigns Python lists to table columns—consider wrapping these in an astropy.table.Column to preserve metadata (and remove once upstream is fixed).
- The manual docstring note reformatting is repetitive; consider automating this with a simple script or adopting a docstring formatter to ensure consistency.
- Since you removed explicit tolerances in test_psf_consistent, verify that the default allclose settings remain strict enough to catch regressions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The astropy Time workaround directly assigns Python lists to table columns—consider wrapping these in an astropy.table.Column to preserve metadata (and remove once upstream is fixed).
- The manual docstring note reformatting is repetitive; consider automating this with a simple script or adopting a docstring formatter to ensure consistency.
- Since you removed explicit tolerances in test_psf_consistent, verify that the default allclose settings remain strict enough to catch regressions.
## Individual Comments
### Comment 1
<location> `aiapy/tests/test_idl.py:92` </location>
<code_context>
# NOTE: Only compare values above some threshold as the
# rest of the PSF is essentially noise
i_valid = np.where(psf_idl > 1e-10)
- assert np.allclose(psf_94[i_valid], psf_idl[i_valid], atol=0.0, rtol=2e-3)
+ assert np.allclose(psf_94[i_valid], psf_idl[i_valid])
</code_context>
<issue_to_address>
**suggestion (testing):** Consider explicitly documenting the rationale for using default tolerances in np.allclose.
Since the test now uses np.allclose's default tolerances, please confirm this is intentional and add a comment explaining why the defaults are suitable. If higher precision is needed, consider specifying the tolerances or noting the reasoning.
</issue_to_address>
### Comment 2
<location> `docs/conf.py:43` </location>
<code_context>
import datetime
import os
import warnings
from pathlib import Path
from packaging.version import Version
# -- Read the Docs Specific Configuration --------------------------------------
# This needs to be done before aiapy/sunpy is imported
on_rtd = os.environ.get("READTHEDOCS", None) == "True"
if on_rtd:
os.environ["SUNPY_CONFIGDIR"] = "/home/docs/"
os.environ["HOME"] = "/home/docs/"
os.environ["LANG"] = "C"
os.environ["LC_ALL"] = "C"
os.environ["PARFIVE_HIDE_PROGRESS"] = "True"
# -- Project information -----------------------------------------------------
# The full version, including alpha/beta/rc tags
from aiapy import __version__
_version = Version(__version__)
version = release = str(_version)
# Avoid "post" appearing in version string in rendered docs
if _version.is_postrelease:
version = release = _version.base_version
# Avoid long githashes in rendered Sphinx docs
elif _version.is_devrelease:
version = release = f"{_version.base_version}.dev{_version.dev}"
is_development = _version.is_devrelease
is_release = not (_version.is_prerelease or _version.is_devrelease)
project = "aiapy"
author = "AIA Instrument Team @ LMSAL"
copyright = f"{datetime.datetime.now(datetime.UTC).year}, {author}" # NOQA: A001
# -- General configuration ---------------------------------------------------
from astropy.utils.exceptions import AstropyDeprecationWarning
from matplotlib import MatplotlibDeprecationWarning
from sunpy.util.exceptions import SunpyDeprecationWarning, SunpyPendingDeprecationWarning
# Need to make sure that our documentation does not raise any of these
warnings.filterwarnings("error", category=SunpyDeprecationWarning)
warnings.filterwarnings("error", category=SunpyPendingDeprecationWarning)
warnings.filterwarnings("error", category=MatplotlibDeprecationWarning)
warnings.filterwarnings("error", category=AstropyDeprecationWarning)
# Wrap large function/method signatures
maximum_signature_line_length = 80
# Add any Sphinx extension module names here, as strings. They can be
# extensions coming with Sphinx (named "sphinx.ext.*") or your custom
# ones.
extensions = [
"sphinx.ext.autodoc",
"sphinx.ext.intersphinx",
"sphinx.ext.todo",
"sphinx.ext.coverage",
"sphinx.ext.inheritance_diagram",
"sphinx.ext.viewcode",
"sphinx.ext.napoleon",
"sphinx.ext.doctest",
"sphinx.ext.mathjax",
"sphinx_automodapi.automodapi",
"sphinx_automodapi.smart_resolver",
"sphinx_changelog",
"sunpy.util.sphinx.generate",
"sphinxext.opengraph",
"sphinx_design",
"sphinx_copybutton",
"matplotlib.sphinxext.plot_directive",
"sphinx_automodapi.automodapi",
"sphinx_automodapi.smart_resolver",
"sphinx_changelog",
"sphinx_gallery.gen_gallery",
]
# Add any paths that contain templates here, relative to this directory.
# templates_path = ["_templates"]
# Register the template for the robots.txt
html_extra_path = ["robots.txt"]
# List of patterns, relative to source directory, that match files and
# directories to ignore when looking for source files.
# This pattern also affects html_static_path and html_extra_path.
exclude_patterns = ["_build", "Thumbs.db", ".DS_Store"]
# The suffix(es) of source filenames.
source_suffix = {".rst": "restructuredtext"}
# The master toctree document.
master_doc = "index"
# Treat everything in single ` as a Python reference.
default_role = "py:obj"
# Enable and configure nitpicky mode
nitpicky = True
# This is not used. See docs/nitpick-exceptions file for the actual listing.
nitpick_ignore = []
with Path("nitpick-exceptions").open() as nitpick_exceptions:
for line in nitpick_exceptions:
if line.strip() == "" or line.startswith("#"):
continue
dtype, target = line.split(None, 1)
target = target.strip()
nitpick_ignore.append((dtype, target))
# -- Options for sphinxext-opengraph ------------------------------------------
ogp_image = "https://raw.githubusercontent.com/sunpy/sunpy-logo/master/generated/sunpy_logo_word.png"
ogp_use_first_image = True
ogp_description_length = 160
ogp_custom_meta_tags = ('<meta property="og:ignore_canonical" content="true" />',)
# -- Options for sphinx-copybutton ---------------------------------------------
# Python Repl + continuation, Bash, ipython and qtconsole + continuation, jupyter-console + continuation
copybutton_prompt_text = r">>> |\.\.\. |\$ |In \[\d*\]: | {2,5}\.\.\.: | {5,8}: "
copybutton_prompt_is_regexp = True
# -- Options for intersphinx extension ---------------------------------------
# Example configuration for intersphinx: refer to the Python standard library.
intersphinx_mapping = {
"astropy": ("https://docs.astropy.org/en/stable/", None),
"cupy": ("https://docs.cupy.dev/en/stable/", None),
"drms": ("https://docs.sunpy.org/projects/drms/en/stable/", None),
"matplotlib": ("https://matplotlib.org/stable", None),
"numpy": ("https://numpy.org/doc/stable/", (None, "http://www.astropy.org/astropy-data/intersphinx/numpy.inv")),
"parfive": ("https://parfive.readthedocs.io/en/stable/", None),
"pyerfa": ("https://pyerfa.readthedocs.io/en/stable/", None),
"python": ("https://docs.python.org/3/", (None, "http://www.astropy.org/astropy-data/intersphinx/python3.inv")),
"reproject": ("https://reproject.readthedocs.io/en/stable/", None),
"scipy": (
"https://docs.scipy.org/doc/scipy/reference/",
(None, "http://www.astropy.org/astropy-data/intersphinx/scipy.inv"),
),
"skimage": ("https://scikit-image.org/docs/stable/", None),
"sunpy": ("https://docs.sunpy.org/en/stable/", None),
}
# -- Options for HTML output -------------------------------------------------
# The theme to use for HTML and HTML Help pages. See the documentation for
# a list of builtin themes.
html_theme = "sunpy"
# Render inheritance diagrams in SVG
graphviz_output_format = "svg"
graphviz_dot_args = [
"-Nfontsize=10",
"-Nfontname=Helvetica Neue, Helvetica, Arial, sans-serif",
"-Efontsize=10",
"-Efontname=Helvetica Neue, Helvetica, Arial, sans-serif",
"-Gfontsize=10",
"-Gfontname=Helvetica Neue, Helvetica, Arial, sans-serif",
]
# Add any paths that contain custom static files (such as style sheets) here,
# relative to this directory. They are copied after the builtin static files,
# so a file named "default.css" will overwrite the builtin "default.css".
# html_static_path = ["_static"]
# By default, when rendering docstrings for classes, sphinx.ext.autodoc will
# make docs with the class-level docstring and the class-method docstrings,
# but not the __init__ docstring, which often contains the parameters to
# class constructors across the scientific Python ecosystem. The option below
# will append the __init__ docstring to the class-level docstring when rendering
# the docs. For more options, see:
# https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autoclass_content
autoclass_content = "both"
# -- Sphinx Gallery ------------------------------------------------------------
from sunpy_sphinx_theme import PNG_ICON
# JSOC email os env
# see https://github.com/sunpy/sunpy/wiki/Home:-JSOC
os.environ["JSOC_EMAIL"] = "jsoc@sunpy.org"
sphinx_gallery_conf = {
"backreferences_dir": Path("generated") / "modules",
"filename_pattern": "^((?!skip_).)*$",
"examples_dirs": Path("..") / "examples",
"gallery_dirs": Path("generated") / "gallery",
"matplotlib_animations": True,
"default_thumb_file": PNG_ICON,
"abort_on_example_error": False,
"plot_gallery": "True",
"remove_config_comments": True,
"doc_module": ("aiapy"),
"only_warn_on_example_error": True,
}
</code_context>
<issue_to_address>
**issue (code-quality):** Don't assign to builtin variable `copyright` ([`avoid-builtin-shadow`](https://docs.sourcery.ai/Reference/Default-Rules/comments/avoid-builtin-shadow/))
<br/><details><summary>Explanation</summary>Python has a number of `builtin` variables: functions and constants that
form a part of the language, such as `list`, `getattr`, and `type`
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
```python
list = [1, 2, 3]
```
However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as `integers`.
In a pinch, `my_list` and similar names are colloquially-recognized
placeholders.</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Bump the project’s minimum Python requirement to 3.11 and update all CI, tox, and linter configurations to use Python 3.13; standardize Sphinx note formatting in docstrings; work around an Astropy Time conversion segfault with NumPy 2.3.0; and adjust test and configuration files accordingly.
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: