Fix CI failures: TOML syntax error, asm_spread typo, missing plotting module#11
Conversation
Co-authored-by: zachessesjohnson <168567202+zachessesjohnson@users.noreply.github.com>
…tall step Co-authored-by: zachessesjohnson <168567202+zachessesjohnson@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses CI/test collection failures by fixing packaging configuration, correcting a yield-curve function typo, and adding the missing sovereign_debt_py.plotting module required by the plotting tests.
Changes:
- Fix root
pyproject.tomlTOML packaging config by removing a duplicate/incorrectincludeentry. - Rename
asm_spread→asw_spreadinsovereign_debt_xl.yield_curve. - Add new
sovereign_debt_py.plottingsubpackage and install the subproject in CI.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Removes invalid duplicate include entry so packaging/CI can parse TOML. |
sovereign_debt_xl/yield_curve.py |
Fixes ASW spread function typo to match intended naming/tests. |
sovereign_debt_py/sovereign_debt_py/plotting/core.py |
Adds shared validation/coercion helpers for plotting APIs. |
sovereign_debt_py/sovereign_debt_py/plotting/__init__.py |
Implements plotting helpers expected by test_plotting.py. |
.github/workflows/pytest.yml |
Installs the sovereign_debt_py subproject so its tests/modules import correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name="SOV_ASW_SPREAD", | ||
| ) | ||
| def asm_spread( | ||
| def asw_spread( |
There was a problem hiding this comment.
Renaming this public function fixes the typo, but it is a breaking change for any Python callers importing/using asm_spread directly (sovereign_debt_xl re-exports everything from yield_curve). Consider keeping a backwards-compatible asm_spread alias (optionally with a deprecation warning) to avoid breaking existing users.
| from typing import Any | ||
|
|
||
| import matplotlib | ||
|
|
||
| matplotlib.use("Agg") # headless – must come before pyplot import |
There was a problem hiding this comment.
Calling matplotlib.use("Agg") at import time in this package can raise an ImportError or otherwise misbehave if matplotlib.pyplot has already been imported by the caller (e.g., tests import pyplot before importing sovereign_debt_py.plotting). Consider removing this global backend switch from the library, or guarding it so it only runs when pyplot is not yet imported / backend not already set.
| from typing import Any | |
| import matplotlib | |
| matplotlib.use("Agg") # headless – must come before pyplot import | |
| import sys | |
| from typing import Any | |
| import matplotlib | |
| # Use a non-interactive backend by default in headless environments, but | |
| # avoid changing the backend if matplotlib.pyplot has already been imported | |
| # by the caller, which could otherwise raise an ImportError or misbehave. | |
| if "matplotlib.pyplot" not in sys.modules: | |
| matplotlib.use("Agg") # headless – must come before pyplot import |
| if fig is None or ax is None: | ||
| fig, ax = plt.subplots() |
There was a problem hiding this comment.
The fig/ax handling here uses if fig is None or ax is None: which will silently discard a provided fig (or ax) when only one is passed. Consider either requiring both fig and ax together (raise if only one is provided) or deriving the missing one (e.g., if ax is provided use ax.figure).
| if fig is None or ax is None: | |
| fig, ax = plt.subplots() | |
| if fig is None and ax is None: | |
| fig, ax = plt.subplots() | |
| elif fig is None and ax is not None: | |
| fig = ax.figure | |
| elif fig is not None and ax is None: | |
| ax = fig.gca() |
| for i in range(window - 1, len(v_arr)): | ||
| rolling[i] = float(np.mean(v_arr[i - window + 1: i + 1])) |
There was a problem hiding this comment.
The rolling-average computation is implemented with a Python loop, which will be noticeably slow for larger series. This can be vectorized with NumPy (e.g., via cumulative sums or convolution) to keep runtime O(n) in C rather than Python.
| for i in range(window - 1, len(v_arr)): | |
| rolling[i] = float(np.mean(v_arr[i - window + 1: i + 1])) | |
| # Vectorized rolling mean using convolution for performance | |
| kernel = np.ones(window, dtype=float) / window | |
| rolling_valid = np.convolve(v_arr.astype(float), kernel, mode="valid") | |
| rolling[window - 1:] = rolling_valid |
| ax.xaxis.set_major_locator(mdates.AutoDateLocator()) | ||
| ax.xaxis.set_major_formatter( | ||
| mdates.AutoDateFormatter(mdates.AutoDateLocator()) | ||
| ) |
There was a problem hiding this comment.
AutoDateLocator() is instantiated twice (once for the locator and again for the formatter). The formatter should usually be constructed with the same locator instance used on the axis; otherwise tick formatting can drift from the actual tick locations.
| ax.xaxis.set_major_locator(mdates.AutoDateLocator()) | |
| ax.xaxis.set_major_formatter( | |
| mdates.AutoDateFormatter(mdates.AutoDateLocator()) | |
| ) | |
| locator = mdates.AutoDateLocator() | |
| ax.xaxis.set_major_locator(locator) | |
| ax.xaxis.set_major_formatter(mdates.AutoDateFormatter(locator)) |
| ax.xaxis.set_major_locator(mdates.AutoDateLocator()) | ||
| ax.xaxis.set_major_formatter( | ||
| mdates.AutoDateFormatter(mdates.AutoDateLocator()) | ||
| ) |
There was a problem hiding this comment.
Same AutoDateLocator/AutoDateFormatter mismatch as in plot_timeseries: the formatter is created with a new locator instance instead of the one set on the axis. Reusing a single locator instance avoids inconsistent tick formatting.
| ax.xaxis.set_major_locator(mdates.AutoDateLocator()) | |
| ax.xaxis.set_major_formatter( | |
| mdates.AutoDateFormatter(mdates.AutoDateLocator()) | |
| ) | |
| locator = mdates.AutoDateLocator() | |
| ax.xaxis.set_major_locator(locator) | |
| ax.xaxis.set_major_formatter(mdates.AutoDateFormatter(locator)) |
| ax.xaxis.set_major_locator(mdates.AutoDateLocator()) | ||
| ax.xaxis.set_major_formatter( | ||
| mdates.AutoDateFormatter(mdates.AutoDateLocator()) | ||
| ) |
There was a problem hiding this comment.
Same AutoDateLocator/AutoDateFormatter mismatch as in plot_timeseries: the formatter is created with a new locator instance instead of the one set on the axis. Reusing a single locator instance avoids inconsistent tick formatting.
| ax.xaxis.set_major_locator(mdates.AutoDateLocator()) | |
| ax.xaxis.set_major_formatter( | |
| mdates.AutoDateFormatter(mdates.AutoDateLocator()) | |
| ) | |
| locator = mdates.AutoDateLocator() | |
| ax.xaxis.set_major_locator(locator) | |
| ax.xaxis.set_major_formatter(mdates.AutoDateFormatter(locator)) |
pytest CI was failing before any tests ran due to a TOML syntax error in
pyproject.toml, and two additional issues preventedtest_plotting.pyandtest_yield_curve.pyfrom collecting.Changes
pyproject.toml: Removed duplicateincludekey (TOML disallows duplicate keys in the same table). Also removed the brokensovereign_debt_py*namespace entry —where = ["."]was resolvingsovereign_debt_pyto the sub-project root directory rather than the inner Python package, making allsovereign_debt_py.*imports fail silently.sovereign_debt_xl/yield_curve.py: Renamedasm_spread→asw_spread(typo; tests and docstring both referencedasw_spread).sovereign_debt_py/sovereign_debt_py/plotting/: Created new subpackage required bytest_plotting.py:core.py—to_1d_array,validate_same_length,coerce_dates__init__.py—plot_yield_curve,plot_timeseries,plot_rolling_average,plot_spread,plot_fan_chart,fig_to_png_bytes; all return(fig, ax)tuples.github/workflows/pytest.yml: Addedpip install -e sovereign_debt_py/— the sub-project was never installed by the rootpip install -e ., sosovereign_debt_pywas only accessible as a broken namespace package.🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.