df.streamdf: streamTrack method + deprecate calc_stream_lb (and Rnorm/Vnorm)#878
Open
jobovy wants to merge 11 commits into
Open
df.streamdf: streamTrack method + deprecate calc_stream_lb (and Rnorm/Vnorm)#878jobovy wants to merge 11 commits into
jobovy wants to merge 11 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #878 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 225 225
Lines 33875 33893 +18
Branches 710 708 -2
=======================================
+ Hits 33848 33866 +18
Misses 27 27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…Vnorm
Unify the streamdf and streamspraydf track interfaces by adding a
streamTrack() method on streamdf that returns a StreamTrack view built
from streamdf's analytically computed action-angle track (no particle
fitting needed). _parse_track_dim is replumbed onto the StreamTrack
mean accessors so plotTrack et al. share the new backend; the spread
plumbing still uses streamdf's eigen-slerp covariance interpolation
(replumbing it would silently change plotTrack(spread=...) numerics).
calc_stream_lb becomes fully redundant given the new StreamTrack
ll/bb/dist/vlos/pmll/pmbb accessors and cov(basis='galsky'). Split into
a private _calc_stream_lb that internal callers (streamdf.__init__,
plotTrack lazy trigger, streamgapdf super shim) use directly, and a
public calc_stream_lb that emits FutureWarning ("deprecated since v1.12,
removed in v1.14") before delegating to the private body. Rnorm/Vnorm
kwargs are modernized from a plain-string warning to the same
FutureWarning convention with explicit removal version.
Covers: StreamTrack instance identity & caching; numerical equality of chunk-grid and fine-grid mean accessors with _ObsTrackXY / _interpolatedObsTrackXY; cov(basis='galcenrect') match at chunk nodes; ll/bb/dist/vlos/pmll/pmbb match _ObsTrackLB and _interpolatedObsTrackLB (streamdf's R0/Zsun/vsun solar convention is threaded through to StreamTrack); custom_transform pass-through enables phi1/phi2/pmphi1/ pmphi2; plotTrack works after the _parse_track_dim replumb in galcen and LB axes with and without spread bands. Two version-gated tests fail when galpy.__version__.release >= 1.14 with cleanup instructions in the failure message — once we cut v1.14, removing the calc_stream_lb FutureWarning shim and the Rnorm/Vnorm kwargs is forced by these tests rather than relying on memory.
Add streamdfstreamtrack.rst (automethod stub mirroring streamspraydfstreamtrack.rst) and a "Unified StreamTrack interface" section in the streamdf tutorial notebook covering the new accessor set (galcen/sky/galsky/customsky), cov(basis='galsky'), and the plotTrack-vs-StreamTrack.plot relationship.
CI runs pytest with -W error::FutureWarning, so the new FutureWarning on streamdf(Rnorm=...) / streamdf(Vnorm=...) was escalated to an error in test_streamgapdf.py impact-direction tests and in test_quantity.py's streamgapdf_method_returntype setup. These tests don't intend to exercise the legacy kwargs — swap them for the modern ro=/vo= form. test_streamdf_RnormWarning and test_streamdf_VnormWarning in test_quantity.py used to assert the legacy plain-string warning message; update them to assert the new FutureWarning (matching v1.12/v1.14 wording). Likewise update test_progenitor_coordtransformparams's coord-transform-conflict probes to trigger via ro=/vo=/R0= rather than the deprecated kwargs (the conflict warnings under test still fire).
When streamTrack() is called on a streamdf constructed with nospreadsetup=True, it must lazily trigger _determine_stream_spread before building the StreamTrack — the only branch of streamTrack() not exercised by the existing fixture (which has the spread precomputed at init). Add a small dedicated test that constructs such a streamdf and asserts both that the cov machinery wasn't populated at init and that streamTrack() populates it.
jobovy
force-pushed
the
streamdf-streamTrack
branch
from
89bbbac to
6c30e60
Compare
streamdf.streamTrack() now attaches a covariance that matches the streamspraydf convention — local perpendicular width only, instead of the full marginal cov used internally for likelihood evaluation. The legacy _allErrCovsXY keeps its current semantics (parallel-angle variance = 1.0 rad², the placeholder for a uniform along-stream distribution) — needed by gaussApprox and plotTrack(spread=N)'s 2D minor-eigenvalue projection. A new _allErrCovsLocalXY (and the fine-grid _interpolatedAllErrCovsLocalXY) is computed with sigangledAngle**2 in the parallel-angle slot instead. This is what streamTrack() now wraps, so track.cov(basis='galsky') and track.plot(spread=N) produce values comparable to streamspraydf's local empirical cov (ratio ~1-2x, vs ~100-200x before). Also builds the StreamTrack from the fine 1001-point _interpolatedThetasTrack grid so track.plot is smooth (the default _ninterp was the chunk-grid length of 11 before). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
streamTrack()'s lazy trigger of _determine_stream_local_spread was only exercised through the single-process path; the else branch (multi.parallel_map) was new and uncovered, dropping patch coverage to 95.45%. Add a streamdf(..., multi=True, nospreadsetup=True) test that calls streamTrack() and verifies the resulting local cov. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `_interpolate_stream_track` lazy trigger in `streamTrack()` only fires for `nosetup=True` streamdfs where the user manually populated the chunk track + spread but skipped the interpolation step. That combination is unsupported in normal flows (the in-init `_determine_stream_spread` path requires the interpolated track), so the line is defensive against a rare manual-construction edge case rather than a regression risk. Marking it `# pragma: no cover` preserves the safety net without dragging in a brittle test that manually orchestrates the streamdf setup steps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The sigma(...) values in the StreamTrack.cov(basis='galsky') cell were left over from the old full-marginal cov (60+ deg in ll/bb, 256 km/s in vlos). With the local-cov change in 4d4814a they are now ~0.5 deg / ~1 km/s — the perpendicular stream width. Also reworks the plotTrack-vs-StreamTrack.plot cell: drop the side-by-side subplots that left an empty axes (plotTrack's internal pyplot.figure() opened a second figure), overlay both on a single axes instead, and refresh the surrounding text to explain the cov semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code review follow-up to the local-cov fix: - Consolidate _determine_stream_spread and _determine_stream_local_spread into a single _determine_stream_spread pass that emits both the full (gaussApprox / plotTrack-spread) and local (streamTrack) covs. Saves the duplicate sigOmega/sigangledAngle evaluations and pulls the eigen-slerp interpolation into a shared _cart_and_interp_cov helper. - streamTrack() now (a) drops the simple= kwarg (only relevant when the spread hasn't been set up, which raises RuntimeError instead of lazily kicking off setup), (b) accepts a per-call custom_sky_transform= override matching streamspraydf.streamTrack's signature, and (c) has a much shorter docstring. - R0 / Zsun / vsun default to None and are sourced from the progenitor Orbit metadata when not explicitly passed; only fall back to the hardcoded legacy defaults (R0=8 kpc, Zsun=20.8 pc, vsun=[-11.1, 8.0*30.24, 7.25] km/s) when the progenitor doesn't carry them either. Existing explicit-value mismatch warnings unchanged. Notebook: clean up the markdown cross-reference link (raw RST was leaking through nbsphinx), trim the verbose plot-cell comment, and re-execute so the sigma(galsky) cell reflects the local-cov values. Tests: replace the now-defunct lazy-spread test with a RuntimeError test; add an equatorial→Galactic custom_sky_transform test that exercises the full Jacobian chain end-to-end (phi1/phi2/pmphi1/pmphi2 should reproduce ll/bb/pmll/pmbb when T is the equatorial→Galactic rotation); add a per-call-override test for streamTrack( custom_sky_transform=...). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
progenitor must be a 3D Orbit for streamdf to work, and Orbit.__init__ always sets _zo and _solarmotion for 3D+ orbits — the 'else 0.0208' / 'else vsun=...' fallbacks I added were defensive against a case that can't occur in practice. They were also the one uncovered line (99.06% patch coverage on the previous push). Remove them so the cov sourcing falls through cleanly to the progenitor's stored values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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
streamdf.streamTrack(simple=_USESIMPLE)returning aStreamTrackview built directly from streamdf's analytic action-angle track — unifies the public interface withstreamspraydf.streamTrackfrom Smooth stream track for streamspraydf models #861._parse_track_dim(used byplotTrack/plotProgenitor) onto the new StreamTrack mean accessors._parse_track_spreadis intentionally not replumbed: streamdf's eigen-slerp covariance interpolation differs from StreamTrack's linear-entry one, and swapping would silently changeplotTrack(spread=...)output.streamgapdf(astreamdfsubclass) continues to read_interpolatedObsTrack*/_allErrCovs*directly.calc_stream_lb(FutureWarning, since v1.12, removed in v1.14) — its entire output is available viastreamTrack().ll/bb/dist/vlos/pmll/pmbbandcov(basis='galsky'). Private/public split keepsstreamdf.__init__,plotTrack's LB lazy trigger, andstreamgapdf.super()._calc_stream_lb()warning-free.Rnorm/Vnormkwarg warnings from plain-stringgalpyWarningtoFutureWarning("since v1.12, removed in v1.14") matching the streamspraydf precedent.galpy.__version__.release >= 1.14, the new tests fail with explicit cleanup instructions — forces the deprecation to be honored at the version bump rather than relying on memory.streamdfstreamtrack.rstreference page; new "Unified StreamTrack interface" section in the streamdf tutorial notebook (re-run with thepy313kernel).Test plan
pytest tests/test_streamdf.py— 73 passed (9 new + all existing track / spread / closest-point / density / sampling / call / sample / call_marg / etc.).pytest tests/test_streamgapdf.py— 30 passed (super()._calc_stream_lb()shim works).pytest tests/test_streamTrack.py tests/test_streamspraydf.py— 64 passed (no regression in the existing streamTrack path).jupyter nbconvert --execute --inplace doc/source/tutorials/streams/streamdf.ipynbinpy313.track.ll/bb/dist/vlos/pmll/pmbbreproduce_ObsTrackLBtoatol=1e-10;track.x/y/z/vx/vy/vzreproduce_ObsTrackXYexactly at chunk nodes;track.cov(theta_i, basis='galcenrect')reproduces_allErrCovsXY[i]exactly.__version__ = "1.14.0.dev0"locally and confirm the two new deprecation tests fail with the cleanup-prompt message (not part of CI; reviewer can verify).🤖 Generated with Claude Code