Skip to content

Docstrings, documentation, unit tests, bug fixes, and refactors#31

Merged
danielreardon merged 6 commits into
masterfrom
dev
Jul 13, 2026
Merged

Docstrings, documentation, unit tests, bug fixes, and refactors#31
danielreardon merged 6 commits into
masterfrom
dev

Conversation

@danielreardon

@danielreardon danielreardon commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Overview

This PR improves documentation, test coverage, and code quality across scintools, then fixes the bugs surfaced by the review and applies safe refactors. Work was done on the dev branch across four commits.

Documentation

  • Docstrings: every function/method/class now has a NumPy-style docstring (165/165), plus module-level docstrings for all five modules. Verified via AST comparison that the docstring commit changed only docstrings — executable code was byte-identical to master.
  • Online docs (docs/source/): rewrote the placeholder dynspec.rst and simulation.rst pages with real, runnable examples drawn from the example notebooks; filled in acf.rst; and reconciled scint_models.rst/scint_utils.rst/acf.rst signatures with the actual code (e.g. thin_screen → veff_thin_screen, corrected the stale ACF signature, fixed wrong defaults/types, removed stale entries).

Tests

  • New pytest suite under tests/ covering the pure/deterministic functions in scint_utils, scint_models, ththmod, scint_sim, and the dynspec wrapper classes. 74 passing.

Bug fixes

Fixes all 10 confirmed bugs from the review (see CODE_REVIEW.md), plus 4 plausible issues and a plt.colorbar no-op. Highlights:

  • np.complex_ (removed in NumPy 2.0) → np.complex128, which unblocks the entire ACF class and the analytic 2D ACF fit path.
  • fit_arc no longer crashes with TypeError in its default mode (list-vs-float division on constraint).
  • SimDyn.__init__ no longer raises AttributeError (self.header = self.header).
  • get_scint_params 2D-crop copy/paste slip (tmin/tmax → fmin/fmax).
  • BasicDyn mutable defaults / .size-on-list; MatlabDyn except NameError → KeyError; calc_sspec wrong cache attribute; plot_dyn mask; norm_sspec logsteps mask; in-place weight mutation; np.float128 → np.longdouble.
  • New bug found during the fix pass: slow_FT called np.fft.fftshift(SS, axis=0) — numpy's kwarg is axes, so it raised TypeError on every call. Fixed, and made the mid-band reference frequency an optional fref parameter.

Refactors

All behavior-preserving (verified by imports, the test suite, and bit-for-bit output comparison against captured baselines for the untested mosaic code):

  • Consolidated the duplicated svd_model onto a shared svd_reconstruct core.
  • Extracted the ~6×-repeated mosaic weighting block in ththmod.py into a single chunk_mask helper (output verified bit-identical across all 9 affected functions).
  • scale_dyn now reuses get_window; centralized the c = 299792458.0 literal onto scipy.constants.c; removed a large commented-out dead block.

Deliberately left for maintainer review (documented in CODE_REVIEW.md)

  • The scint_velocity error-propagation term (scientific formula needing domain confirmation).
  • Decomposition of the long functions (get_scint_params, fit_arc, norm_sspec, ACF.calc_acf) — deferred until characterization tests exist, to avoid silently altering the untested scientific core.
  • calc_scattered_image fill not merged into interp_nan_2d (different masking semantics); trim_edges' unused remove_short_sub documented as reserved rather than dropped (avoids an API break); make_dynspec placeholder left in place.

🤖 Generated with Claude Code

Generated by Claude Code

claude added 6 commits July 13, 2026 11:43
@claude
Add docstrings, finish documentation, add unit tests, and code review
ccddcd3 
Deployed three sub-agents on the dev branch to improve documentation,
test coverage, and code quality:

Docstrings (docstrings-only; executable code verified byte-identical to
master via AST comparison, and the full test suite passes):
- Added NumPy-style docstrings so all 163 functions/classes across
  dynspec.py, scint_models.py, scint_sim.py, scint_utils.py, and
  ththmod.py are documented, plus module-level docstrings for each.

Online documentation (docs/source):
- Rewrote the placeholder dynspec.rst and simulation.rst pages with real
  usage examples drawn from the example notebooks.
- Reconciled scint_models.rst and scint_utils.rst signatures with the
  code (e.g. thin_screen -> veff_thin_screen; removed stale entries;
  fixed default values and parameter types).

Unit tests (tests/):
- New pytest suite (conftest.py + four test modules) covering pure,
  deterministic functions in scint_utils, scint_models, ththmod, and
  scint_sim. Result: 58 passed, 2 xfailed.
- The 2 xfails flag genuine bugs (np.complex_ removed in NumPy 2.0
  breaks the ACF class; arc_power_curve is an unfinished stub).

Code review (CODE_REVIEW.md):
- Read-only review reporting 10 confirmed bugs and 5 suspected issues,
  plus refactoring opportunities and deprecation notes. No source fixes
  were applied; the report is left for maintainer review.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EG6cv3jHNLYv1TN2YWDqi3
@claude
Fix stale ACF signature in acf.rst documentation
611b02e 
Update the documented ACF class signature and methods in acf.rst to match
the actual scint_sim.ACF implementation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EG6cv3jHNLYv1TN2YWDqi3
@claude
Fix bugs found in code review
307ab24 
Fixes all 10 confirmed bugs from CODE_REVIEW.md, plus 4 of the 5
plausible issues and the plt.colorbar no-op. No refactors applied.

scint_sim.py:
- Replace np.complex_ (removed in NumPy 2.0) with np.complex128 in
  ACF.calc_acf, unblocking the ACF class and the analytic 2D ACF fit.
- Add missing parentheses to plt.colorbar() in plot_pulse.

dynspec.py:
- fit_arc: coerce constraint to a float ndarray so the default
  constraint=[0, np.inf] no longer raises TypeError in the non-lamsteps path.
- SimDyn.__init__: set header from the simulation name instead of reading
  self.header before it exists.
- get_scint_params: fix copy/paste slip that set tmin/tmax (with a frequency
  count) instead of fmin/fmax in the frequency-overflow crop branch.
- BasicDyn: replace mutable default arguments with None and guard on
  len(...) instead of .size (which failed on list defaults); also use
  self.dt (not the raw dt arg) when deriving tobs.
- MatlabDyn: catch KeyError (not NameError) for missing mat-file variables.
- calc_sspec: check the real cache attribute (trapdyn, not trap).
- plot_dyn: restore the nonzero-pixel filter in the intensity-scaling mask.
- norm_sspec: apply masklin (not mask) to the logsteps linear spectrum.
- Use np.longdouble instead of the non-portable np.float128.

scint_models.py:
- tau_sspec_model/dnu_sspec_model: accept an optional weights argument so
  scint_sspec_model no longer raises TypeError.
- arc_power_curve: raise NotImplementedError instead of silently returning
  broken residuals from an empty model.
- effective_velocity_annual: raise KeyError when no inclination parameter is
  supplied instead of hitting a later NameError on INC.
- tau_acf_model/dnu_acf_model: copy weights before zeroing the first element
  so the caller's array is not mutated.

scint_utils.py:
- Use np.longdouble instead of np.float128.

Deliberately left unchanged: the scint_velocity error-propagation term is a
scientific formula needing domain confirmation; flagged in CODE_REVIEW.md.

tests/:
- Convert the two previously-xfailed tests (ACF, arc_power_curve) to passing
  tests, and add regression tests (test_dynspec.py plus new cases in
  test_scint_models.py) covering the fixes. Full suite: 67 passed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EG6cv3jHNLYv1TN2YWDqi3
@claude
Apply code-review refactors and fix slow_FT
b5ecfac 
Refactoring opportunities from CODE_REVIEW.md, all behaviour-preserving
(verified by imports, the unit suite, and bit-for-bit output comparison
against captured baselines for the untested mosaic functions):

- Consolidate the duplicated svd_model: add a shared svd_reconstruct core
  in scint_utils and have both scint_utils.svd_model and ththmod.svd_model
  use it.
- Extract the ~6x copy-pasted mosaic weighting block in ththmod.py into a
  single chunk_mask() helper (mosaic, rotMos, rotInit, rotDer, fullMos,
  fullMosGrad, fullMosHess). Output verified bit-identical.
- Replace the inline window construction in dynspec.scale_dyn with the
  existing scint_utils.get_window.
- Centralize the repeated c = 299792458.0 literal onto scipy.constants.c
  (dynspec.py x4, scint_sim.py x1).
- Remove the large commented-out sspec-method implementation block in
  get_scint_params (keeping the "not implemented" branch behaviour).

New bug found while refactoring and fixed:
- slow_FT called np.fft.fftshift(SS, axis=0); the numpy kwarg is `axes`, so
  the function raised TypeError on every call. Fixed to axes=0, and made the
  hard-coded mid-band reference frequency an optional fref parameter
  (default preserves previous behaviour).

Deliberately left as-is (documented in CODE_REVIEW.md): the long scientific
functions were not decomposed (untested core; risky without characterization
tests), calc_scattered_image's fill was not merged into interp_nan_2d
(different masking semantics), scint_velocity's error term (needs domain
review), trim_edges' unused remove_short_sub (documented as reserved to avoid
an API break), and make_dynspec (documented placeholder).

Adds regression tests for svd_reconstruct/svd_model, chunk_mask, and slow_FT.
Full suite: 74 passed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EG6cv3jHNLYv1TN2YWDqi3
@claude
ci: run pytest suite in GitHub Actions
38e382f 
Add a "Test with pytest" step to the python-package workflow so the unit
test suite under tests/ runs on every push and pull request, across the
Python 3.8-3.10 matrix, in addition to the existing flake8 checks.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EG6cv3jHNLYv1TN2YWDqi3
@claude
ci: install scintools package before running pytest
397b782 
The pytest step failed at collection with "No module named 'scintools'":
CI installed the dependencies but never the package itself, and
`pytest tests/` (unlike `python -m pytest`) does not put the repo root on
sys.path. Install the package in editable mode (pip install -e .) and invoke
the suite as `python -m pytest tests/` so imports resolve on the 3.8-3.10
matrix.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EG6cv3jHNLYv1TN2YWDqi3
@danielreardon
danielreardon merged commit 33a5214 into master Jul 13, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@danielreardon @claude