Skip to content

Commit 2d55afd

Browse files
jobovyclaude
andcommitted
Address 5 review findings
1. (bug) progpot + streamTrack: save the pre-progpot potential as _orig_pot and use it for the track-progenitor integration, avoiding the MovingObjectPotential whose internal orbit is only valid on [-tdisrupt, 0]. 2. (bug) vlos double-scaling: _helio_xv already returns km/s (it pre- multiplies by vo), so the 'vlos' branch of _scale now only attaches units without re-scaling. 3. (docs) Fix docstrings: track_time_range default (now auto-timerange), track_n_dense actual grid size, smoothing description (now GCV), add ntp kwarg docs. 4. (docs) Fix class docstring tp range description (not [-tdisrupt, 0]), add arm_sign and ntp to Parameters section. 5. (cleanup) plot spread band: remove dead s1 variable, gate physical scaling on self._physical only (consistent with _scale), drop the d1-in-cart_idx requirement since the band is only in the d2 direction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent d4fcc07 commit 2d55afd

2 files changed

Lines changed: 54 additions & 30 deletions

File tree

galpy/df/streamTrack.py

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -120,9 +120,11 @@ class StreamTrack:
120120
121121
A StreamTrack holds a smooth mean curve (and optional covariance) in
122122
galactic phase space, parameterized by a progenitor time coordinate
123-
``tp`` in ``[-tdisrupt, 0]``. It is constructed from a cloud of stream
124-
particles (e.g. drawn via ``basestreamspraydf.sample``) plus the
125-
progenitor's orbit.
123+
``tp``. ``tp=0`` is the progenitor today; for a leading arm ``tp > 0``
124+
(future positions the progenitor has yet to reach) and for a trailing
125+
arm ``tp < 0``. The ``tp`` range is determined by the data: it spans
126+
the percentile-trimmed range of the closest-point assignments, bounded
127+
by ``track_time_range`` (typically much smaller than ``tdisrupt``).
126128
127129
Notes
128130
-----
@@ -173,15 +175,21 @@ def __init__(
173175
track_t_grid : array, shape (M,)
174176
The dense time grid on which ``track_prog_cart`` is evaluated.
175177
Used for the closest-point projection.
178+
arm_sign : int, optional
179+
``+1`` for leading arm (tp >= 0), ``-1`` for trailing (tp <= 0).
180+
Controls the sign constraint on the closest-point search window.
181+
ntp : int, optional
182+
Number of binning nodes. Default ``sqrt(N)`` clipped to
183+
``[21, 201]``.
176184
ninterp : int, optional
177185
Resolution of the fine tp grid on which the public track is
178186
stored.
179187
smoothing : None, float, or dict, optional
180188
Smoothing parameter for the mean spline per Cartesian coordinate.
181-
``None`` estimates s from the pairwise noise variance of the
182-
sorted particle sample (Reinsch-like target s = N * sigma^2).
183-
A float sets s for all six coordinates; a dict keyed by
184-
``'x','y','z','vx','vy','vz'`` sets per-coordinate values.
189+
``None`` (default) uses GCV auto-tuning via
190+
``scipy.interpolate.make_smoothing_spline``. A float sets an
191+
explicit ``s`` for ``UnivariateSpline``; a dict keyed by
192+
``'x','y','z','vx','vy','vz'`` sets per-coordinate ``s`` values.
185193
niter : int, optional
186194
Iterations beyond the initial fit. Each iteration reassigns each
187195
particle to the closest point on the current track and refits.
@@ -410,8 +418,12 @@ def _scale(self, val, kind):
410418
return val
411419
if kind == "length":
412420
return val * self._ro * (units.kpc if _APY_UNITS else 1)
413-
if kind == "velocity" or kind == "vlos":
421+
if kind == "velocity":
414422
return val * self._vo * (units.km / units.s if _APY_UNITS else 1)
423+
if kind == "vlos":
424+
# vlos from _vrpmllpmbb is already in km/s (helio_xv pre-
425+
# multiplies by vo); only attach units, don't re-scale.
426+
return val * (units.km / units.s if _APY_UNITS else 1)
415427
if kind == "angle":
416428
return val * (units.rad if _APY_UNITS else 1)
417429
if kind == "degree":
@@ -674,21 +686,18 @@ def plot(self, d1="x", d2="y", spread=0, n=None, **kwargs):
674686
line = pyplot.plot(v1, v2, **kwargs)
675687
if spread > 0 and self._cov_xyz is not None:
676688
cart_idx = {"x": 0, "y": 1, "z": 2, "vx": 3, "vy": 4, "vz": 5}
677-
if d1 in cart_idx and d2 in cart_idx:
678-
i1 = cart_idx[d1]
689+
if d2 in cart_idx:
679690
i2 = cart_idx[d2]
680691
cov = self.cov(tp) # (n_eval, 6, 6)
681-
s1 = numpy.sqrt(numpy.maximum(cov[:, i1, i1], 0.0))
682692
s2 = numpy.sqrt(numpy.maximum(cov[:, i2, i2], 0.0))
683-
# Apply physical scaling if active
684-
if self._physical and self._roSet and d1 in ("x", "y", "z"):
685-
s1 = s1 * self._ro
686-
if self._physical and self._roSet and d2 in ("x", "y", "z"):
687-
s2 = s2 * self._ro
688-
if self._physical and self._voSet and d1 in ("vx", "vy", "vz"):
689-
s1 = s1 * self._vo
690-
if self._physical and self._voSet and d2 in ("vx", "vy", "vz"):
691-
s2 = s2 * self._vo
693+
# Scale covariance sigma the same way _scale handles the
694+
# coordinate values (check _physical only, consistent with
695+
# the coordinate accessors).
696+
if self._physical:
697+
if d2 in ("x", "y", "z"):
698+
s2 = s2 * self._ro
699+
elif d2 in ("vx", "vy", "vz"):
700+
s2 = s2 * self._vo
692701
color = line[0].get_color() if line else None
693702
pyplot.fill_between(
694703
v1,

galpy/df/streamspraydf.py

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ def __init__(
133133
else:
134134
self._center = None
135135
if progpot is not None:
136+
self._orig_pot = self._pot # save pre-progpot for streamTrack
136137
progtrajpot = MovingObjectPotential(
137138
orbit=self._progenitor,
138139
pot=progpot,
@@ -248,18 +249,25 @@ def streamTrack(
248249
the value set at initialization.
249250
track_time_range : float or Quantity, optional
250251
Half-range (symmetric about tp=0) of the finely-integrated
251-
progenitor orbit used for closest-point matching and as the
252-
public tp grid. Default is ``0.1*tdisrupt``. Must be larger than
253-
the stream's spatial extent in orbital-time units.
252+
progenitor orbit used for closest-point matching. Default is
253+
data-driven: ``8 * d_max / |v_prog|`` clamped to ``[1,
254+
tdisrupt]``, where ``d_max`` is the farthest particle's
255+
distance from the progenitor.
254256
track_n_dense : int, optional
255257
Number of time points on the finely-integrated progenitor
256-
orbit. Default 10001.
258+
orbit. The actual grid has ``2 * (track_n_dense+1)//2 - 1``
259+
points (forward + backward with shared t=0). Default 10001.
260+
ntp : int, optional
261+
Number of binning nodes. Default ``sqrt(N)`` clipped to
262+
``[21, 201]``.
257263
ninterp : int, optional
258264
Resolution of the public fine-grid track arrays. Default 1001.
259265
smoothing : None, float, or dict, optional
260-
Smoothing spline ``s`` parameter for the mean-track fit.
261-
``None`` estimates s from the pairwise noise variance of the
262-
sorted particle sample per coordinate.
266+
Smoothing parameter. ``None`` (default) uses GCV auto-tuning
267+
via ``scipy.interpolate.make_smoothing_spline``. A float sets
268+
an explicit ``s`` value for ``UnivariateSpline``; a dict
269+
keyed by ``'x','y','z','vx','vy','vz'`` sets per-coordinate
270+
``s`` values.
263271
niter : int, optional
264272
Iterations beyond the initial fit. Each iteration reassigns
265273
particles to the closest point on the current track.
@@ -323,15 +331,22 @@ def streamTrack(
323331
# Build a finely-sampled progenitor phase-space array spanning
324332
# [-T, +T] around the present day. Integrate forward and backward
325333
# separately from the progenitor's present-day state, then combine.
326-
half_dense = max(2, int(track_n_dense) // 2)
334+
# Use the BASE potential (without progpot's MovingObjectPotential)
335+
# because the MovingObjectPotential's internal progenitor was only
336+
# integrated on [-tdisrupt, 0] and would give wrong or erroring
337+
# results for positive times.
338+
_track_pot = self._pot
339+
if hasattr(self, "_orig_pot"):
340+
_track_pot = self._orig_pot
341+
half_dense = (int(track_n_dense) + 1) // 2
327342
t_back = numpy.linspace(0.0, -track_time_range, half_dense)
328343
t_fwd = numpy.linspace(0.0, track_time_range, half_dense)
329344
prog_back = self._orig_progenitor()
330345
prog_back.turn_physical_off()
331-
prog_back.integrate(t_back, self._pot)
346+
prog_back.integrate(t_back, _track_pot)
332347
prog_fwd = self._orig_progenitor()
333348
prog_fwd.turn_physical_off()
334-
prog_fwd.integrate(t_fwd, self._pot)
349+
prog_fwd.integrate(t_fwd, _track_pot)
335350
# Combine, skipping the t=0 duplicate
336351
track_t_grid = numpy.concatenate([t_back[::-1], t_fwd[1:]])
337352
track_prog_cart = numpy.column_stack(

0 commit comments

Comments
 (0)