Mm rework lingfeng into Dev - #26
Open
wei-lingfeng wants to merge 315 commits into
Open
Conversation
…ab/flystar into motion_model_mjh merge
…zed list_times calculation; Fixed deepcopy problem of astropy Table; Changed all meta to list as astropy does not support numpy array in meta data; Changed default dr_tol and dm_tol for MosaicSelfRef and MosaicToRef to one iteration
…within atol and rtol, instead of precise masking! Add uncertainty columns as infinity
…ncertainty function does not need it; Removed the extra 1/sqrt(N) in standard deviation
…d corresponding test functions
…when all motion model is Fixed; Fixed nan t0 when all motion model is Fixed; Fixed star name length truncation warning message
…; Reused multiprocessing pool across the whole call instead of per motion-model group; Added organize_motion_models() and fixed issubclass() TypeError on list input; Added xe/ye/me to plotly_stars hover labels Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd fit_motion_models - Add Fixed.run_fit_batch() for vectorized batch fitting, used by fit_motion_models whenever a motion model supports it - Optimize fit_motion_models' valid_xy/n_fit computation to avoid numpy.ma overhead and per-star set() loops - Fix xe/ye/me defaulting to nan instead of inf for invalid entries - Add progress-bar print for the vectorized Fixed fitting path - In update_ref_table_aggregates, route stars with <=1 valid epoch straight to combine_lists_xym instead of forcing all stars through fit_motion_models whenever any star needs a non-Fixed model; stars needing fit_motion_models' missing-error unit-weight fallback are kept out of that fast path so results stay identical Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_motion_models paths update_ref_table_aggregates's "simple" star branch was hardcoded to weighted_m=False to match fit_motion_models's magnitude-combining logic, which previously always averaged unweighted regardless of the weights_col condition due to an inverted check. Now that the inverted check is fixed (weights_col='me' when it exists), route simple stars through the same correctly-weighted averaging so both paths agree. This also changes m0_err for single-epoch stars: previously their unweighted "spread of residuals" was always exactly 0 (nothing to spread over 1 point) and got forced to inf, silently excluding them from error-based test checks. Weighted averaging instead propagates that star's own per-epoch uncertainty, giving a real (if large) m0_err. Updated test_MosaicSelfRef_vel/_tconst's m0_err threshold and test_MosaicToRef_acc's acceleration tolerance to account for these now more accurate, if occasionally larger, values. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
astropy's Table.__setstate__ reconstructs a table via self.__init__(columns, meta=meta), passing the columns positionally instead of as name=/x=/y=/m= keywords. StarList.__init__'s required- argument check only looked at kwargs, so every pickle round-trip (and any StarList(existing_table) call) warned about missing required arguments even though the columns were all present. Now also accept a single positional Table-like argument (Table, dict/OrderedDict of Columns, list of Columns) that already carries name/x/y/m. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…6841657024 Add claude GitHub actions 1786841657024
- apply_mag_lim_via_use_in_trans: NaN comparisons are always False, so a reference star with a non-finite m0 could never be excluded by the magnitude-range check and would flood into use_in_trans, corrupting the magnitude-zeropoint transform fit. - fit_motion_models: default t0 mixed a masked weights array with plain np.average (not np.ma.average), silently giving t0=NaN for some multi-epoch stars and crashing curve_fit for Linear. Fixed with the same np.ma.average(...).filled(np.nan) pattern already used elsewhere in align.py for the same failure mode. - combine_lists: a star whose every epoch has an invalid raw uncertainty (e.g. missing/invalid me/xe/ye everywhere) but at least one valid value now falls back to a nominal uncertainty of 1 (in that column's own units, before any flux conversion) on its valid epochs, i.e. an unweighted mean, instead of discarding the value as nan -- mirrors fit_motion_models' existing xe/ye=1 fallback for the same situation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…n layers MosaicSelfRef/MosaicToRef gain an inherit_n_detect parameter (default True). When an input starlist already has its own 'n_detect' column (e.g. it's itself the output of a previous, lower-level align pass), that starlist's own per-star n_detect value is used -- instead of counting 1 -- as its contribution to this mosaic's n_detect, tracked per-list in a new 'n_detect_list' column. Starlists without their own 'n_detect' still contribute 1 per detection, same as before. This lets n_detect reflect the total number of raw detections a star represents, however many alignment layers deep. StarTable.detections() gains an optional weight_col argument to sum a per-list column instead of counting valid (x, y) as 1; copy_over_values excludes the 1D 'n_detect' aggregate from its generic by-name column copy and instead writes into 'n_detect_list' explicitly, since a starlist's own 'n_detect' would otherwise collide with the aggregate once detections() has run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…inf->finite bug combine_lists' weighted branch previously patched a fake weight into the raw uncertainty array for stars with no usable weight anywhere, then had to remember to force the reported error back to inf afterward -- fragile, and this exact class of bug (a fabricated finite error silently reaching a real output column) was already found and fixed in fit_motion_models this session. Rewrote it so std is always derived directly from a wgt_sum built only from real, known uncertainties (naturally inf via 1/0 when none exist, no override needed), while the fallback value for those stars is now a single, direct line: a plain mean of their valid epoch(s), same as the unweighted branch already computes. This also removes an unintended bias the old nominal-error scheme introduced for magnitude columns (weighting fallback epochs by assumed equal *magnitude* uncertainty, which the flux conversion turned into favoring the fainter star) in favor of a plain, unbiased flux-space mean. Added test_combine_lists_weight_fallback covering fully-weighted, partially-weighted, single/multi-epoch fallback, no-data, and composite cases for both a plain and a magnitude (flux-space) column, plus the untouched unweighted branch, all asserting inf/nan exactly rather than loose bounds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…>nan bug Linear's existing use_scipy=False path already had a closed-form (normal equations) solution, but ran it one star at a time with a full (n_epochs, n_epochs) diagonal weight matrix and np.linalg.pinv/matrix_rank per star -- wasteful for what's always exactly a 2x2 system, and still required per-star looping or multiprocessing to cover many stars. run_fit_batch computes the same closed form across a whole group of stars at once via vectorized weighted sums and a closed-form 2x2 inverse, with no per-star Python loop and no multiprocessing needed. Validated against the existing per-star run_fit(use_scipy=False) across 18,000 synthetic cases (ragged epoch counts, forced-singular and insufficient-data stars, both weighting schemes, absolute_sigma on/off): 0 mismatches on params/errors/chi2 for every well-posed case. Along the way, found and fixed a real bug the reference implementation didn't have an equivalent for: the absolute_sigma=False chi2 rescaling (param_errs *= sqrt(reduced_chi2)) silently turned a singular fit's correct inf error into nan, since inf * sqrt(nan) == nan. Fixed by re-applying the singular/insufficient-data overrides as an unconditional final step. One deliberate difference from the per-star path: a singular fit's param values are now fill_value rather than np.linalg.pinv's arbitrary minimum-norm artifact, since the reported error is inf either way and nothing should be trusting that value regardless. fit_motion_models' run_fit_batch call site now threads through a per-group fixed_params_dict (sliced from the same array/scalar params construction already used for the per-star path) so Linear's required t0 reaches it; Fixed.run_fit_batch gained a matching (unused) fixed_params_dict parameter for interface consistency. Also gave Empty a trivial run_fit_batch (it never looks at data -- always fill_value/inf). Nearly every real mosaic has some Empty stars, and without this its lack of a batch path alone was forcing a full multiprocessing pool to spawn -- paying real per-worker spawn cost -- just to run an O(1) fill operation one star at a time. With Empty, Fixed, and Linear all batched, processes=3 dropped from 7.07s to 2.68s on a real dataset (matching processes=1's 2.66s), confirming the pool no longer spins up when no group actually needs per-star fitting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…pe loop This loop iterates over the handful of possible motion model types, not per-star, so a progress bar here was never meaningful. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adding new-star rows built a whole parallel StarTable for the new rows, then vstack()-ed it onto the growing ref_table -- vstack (and the intermediate table it needs) transiently holds the old table, the new table, and its own freshly-concatenated result all in memory at once, for every column simultaneously. That roughly doubled peak memory on every single "add new stars" step, which dominates total memory use for a mosaic that grows into the millions of rows across many starlists. Now concatenates each column directly and drops the old column's reference immediately after, so only one column's old+new data is ever resident at a time, then builds the new table via StarTable(..., copy=False) so the already-correct, already-concatenated arrays aren't copied again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…weighting Three changes across startables.py/motion_model.py, found and validated while auditing memory usage for a large (18-starlist, millions-of-stars) mosaic: 1. StarTable.__init__ gains an opt-in copy=True/False parameter (default True, so all existing callers are unaffected) so callers who already own fresh, uncopied arrays can avoid an unnecessary duplicate copy. Also rewrote it to build every column upfront and construct the table in a single call, instead of add_column()-ing columns one at a time -- add_column() turned out to be dramatically more expensive per call than passing every column to the constructor together (confirmed empirically: ~1.2s and +4.6GB for ~29 columns via a loop of add_column() calls at ~1.4M rows, vs ~0.001s and ~0GB for the same columns built in one call). 2. fit_motion_models applied select_stars only after an O(N_stars) data- prep step (masked-array copies of the whole table's x/y/xe/ye), regardless of how few stars actually needed (re)fitting. Since this function is called once per starlist as a mosaic grows, that cost scaled with N_lists x N_stars -- likely the dominant memory/time cost at many-starlist scale. Now slices down to just the selected rows first (bounding cost to len(select_stars)), runs the existing logic unmodified on that smaller table, then scatters results back. 3. Fixed a real, independent correctness bug surfaced while validating the above: if a star's motion_model_used changes to a simpler model across successive fit_motion_models calls (e.g. Linear -> Fixed, because it now matches fewer epochs than before -- this can happen because reset_ref_values()+re-matching rebuilds a star's epoch data from scratch multiple times per align() call, so epoch count isn't monotonic), the old model's now-irrelevant params (e.g. vx/vy) were never reset and stayed stale. Alignment itself was never affected (infer_positions and the fitting write-back already correctly gate on each star's own motion_model_used), but the stale value would corrupt any direct downstream use of those columns. Now resets any param not belonging to a star's current model to fill_value/inf right after (re)classification. 4. Consolidated the "turn an uncertainty array into a safe inverse- variance weight (0 for nan/inf/zero/overflow instead of corrupting the sum)" logic -- previously reimplemented identically in Fixed.run_fit_batch, Linear.run_fit_batch, and combine_lists -- into one shared motion_model.weight_from_sigma(). Validated: full test suite unaffected (only the 2 known pre-existing, unrelated failures); ~13,000 synthetic Linear.run_fit_batch cases against the per-star reference (0 mismatches); a dedicated select_stars-vs-full- table correctness test including a star that changes motion model between calls; StarTable copy=True/False aliasing behavior verified directly; real dataset runs end-to-end with unchanged results. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…shold to motion-model fitting Found while profiling a large (18-starlist, ~1.4M-star) mosaic for further runtime/memory wins on top of the previous round: 1. match.py's match() gains a `workers` parameter (default 1, unchanged behavior) controlling scipy KDTree.query_ball_point's thread count. Verified this doesn't affect the delicate dm_min==dr_min tie-break (which depends on within-list neighbor order): workers=1 vs workers=-1 give identical neighbor lists, order included, across dense/duplicate- point edge cases plus a full end-to-end run (all 37 ref_table columns byte-identical). align.py's MosaicSelfRef/MosaicToRef expose this as match_workers (default 1, not -1) -- most production runs of this code share a machine with other users, so grabbing all cores by default would be antisocial; callers who want the speedup opt in explicitly. 2. startables.py: combine_lists() and fit_motion_models() built list_indices as an arange() array even when no lists were masked, forcing every downstream x[:, list_indices]-style slice into a fancy- index copy on top of the copy=True/deepcopy() calls that already copy. Now uses slice(None) in the no-masking case, removing one of two redundant full (N_stars, N_times) copies for x, y, xe, ye, and t. Verified via isolated A/B: 35/37 output columns byte-identical; m0/ m0_err differ at the 1-2 ULP level (max abs diff 7e-15), a harmless floating-point summation-order artifact from view-vs-copy memory layout, not a logic change. 3. fit_motion_models built one Python dict per star (fixed_params_stars) unconditionally for all N_stars, even though it's only ever read for stars whose motion model lacks a vectorized run_fit_batch (or when bootstrapping) -- on this benchmark 83.6% of stars (Fixed) never touched it. Now built lazily, only for that subset, mirroring the same pattern already used for the neighboring unmasked_idx array. Verified byte-identical output (this one has no floating-point side effect at all, unlike #2). 4. Added mp_star_threshold (default 100_000), threaded through fit()/match_and_transform()/update_ref_table_aggregates()/ calc_bootstrap_errors()/fit_motion_models(). Multiprocessing Pool creation for motion-model fitting is gated on the number of stars actually needing the non-vectorized path (non_batch_star_idxs) meeting this threshold, even if processes > 1 was requested -- below it, the fixed cost of spawning workers and shipping the shared data arrays isn't worth paying. Break-even measured empirically (Acceleration model, no run_fit_batch) between 20,000 and 100,000 stars on a 10-core machine, so 100_000 is a conservative default. Confirmed the 18- starlist benchmark never exercises this path at all -- Empty/Fixed/ Linear all already have run_fit_batch, so multiprocessing measurably made zero difference there (104.65s vs 105.51s, processes=4 vs 1). Validated throughout: relevant test files pass (test_startable.py, test_align.py minus one pre-existing unrelated failure; full suite previously confirmed at 44 passed / 2 known pre-existing failures), plus dedicated small-N tests confirming the Pool is/isn't created as expected and gives identical results either way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds .readthedocs.yaml (Sphinx build via docs/conf.py, package installed
with the docs extra) so this repo can be built on readthedocs.org.
Getting there required fixing three real, pre-existing bugs -- none of
these are specific to Read the Docs, they'd bite anyone doing a fresh
install on a current Python:
1. pyproject.toml's [build-system] pinned cython==0.29.14,
extension-helpers, and oldest-supported-numpy -- leftover astropy
package-template boilerplate for a package with zero .pyx/C
extensions. That pinned Cython imports the stdlib `cgi` module,
which was removed in Python 3.13, so `pip install .` failed outright
on any current Python. Removed all three; only setuptools/
setuptools_scm/wheel are actually needed.
2. pyproject.toml's dependencies list was missing scipy, matplotlib,
tqdm, joblib, and pandas -- all imported unconditionally at module
level in core files (startables.py imports pandas, parallax.py
imports joblib, align.py/plots.py import matplotlib, etc.), so a
plain `pip install flystar` didn't actually install what the code
needs to run. Added them, plus docs/test optional-dependency groups
(sphinx-astropy / pytest-astropy) matching what setup.cfg already
declared.
3. setup.cfg's github_project was left at the template default
("astropy/astropy"), which conf.py uses unconditionally to build
github_issues_url -- every issue-number reference in the built docs
would have linked into astropy's own issue tracker. Fixed to
MovingUniverseLab/flystar. Also removed a dead [options.entry_points]
entry pointing at a template placeholder module (packagename.example_mod)
that was never filled in.
Validated by installing into two independent fresh venvs (editable and
the exact non-editable `pip install .[docs]` Read the Docs itself runs)
and running `sphinx-build -b html` end to end: succeeds, 92 warnings (all
pre-existing docstring formatting issues, e.g. RST title-underline
length -- unrelated to this change and not addressed here).
Also gitignored docs/api/, the autosummary-generated stub .rst files
Sphinx writes into the source tree on every build.
Co-Authored-By: Claude Sonnet 5 <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
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
This merges
mm_rework_lingfengintodev. #25 documents the original set of semantic/behavioral changes that differentiate this branch frommm_rework— uncertainty handling (error propagation vs. weighted variance),Parallax's point requirement, weight-calculation precision,fit_star_idxsoverwrite behavior, and theabsolute_sigmadefault — see that PR for the full list and a worked example.On top of that baseline, this branch also carries a substantial follow-on pass of correctness, performance, and memory fixes, summarized below.
Correctness fixes
97e2c7e): a non-finitem0could never be excluded by the magnitude-range check (NaN comparisons are alwaysFalse), letting bad reference stars corrupt the magnitude-zeropoint transform;fit_motion_models' defaultt0could silently come outNaNfor some multi-epoch stars from a masked/plain-average mismatch;combine_listsnow falls back to a nominal unweighted mean instead of discarding a star's only valid measurement when every epoch's uncertainty is invalid.cb7dbd3): fixed an invertedweights_colcheck that madefit_motion_modelsalways average magnitudes unweighted; routed "simple" (≤1 valid epoch) stars inupdate_ref_table_aggregatesthrough the same corrected weighting so both paths agree. Single-epoch stars now get a real (if large)m0_errinstead of a forcedinf. This resolves thetest_MosaicSelfRef_velfailure noted as a known issue in Update mm_rework to match mm_rework_lingfeng for comparison #25.9a0f8c5):absolute_sigma=False's chi2 rescaling could turn a singular fit's correctinferror intonan(inf * sqrt(nan) == nan); fixed by reapplying the singular/insufficient-data override unconditionally at the end.combine_lists(eba8435): removed a fragile "patch a fake weight in, then remember to force the error back toinf" pattern in favor of derivingstddirectly from real weights only (naturallyinfvia1/0, no override needed).StarListwarning (2bc7926): every pickle round-trip (astropy passes columns positionally to__init__) incorrectly warned about missing required arguments.9287e51): a star that dropped to a simpler motion model (e.g. Linear → Fixed, since epoch counts aren't monotonic acrossalign()iterations) kept stale params (vx/vy) from its old model indefinitely; now reset tofill_value/infright after reclassification.Performance & memory
Fixed(cea7916),Linear(9a0f8c5), and a trivial batch path forEmpty(9a0f8c5) —fit_motion_modelsnow fits a whole group of stars in one vectorized pass instead of one star at a time, whenever a motion model supports it. On a real dataset,processes=3dropped from 7.07s to 2.68s (matching serial's 2.66s) once all three common models were batched, since a multiprocessing pool no longer needs to spin up just to run an O(1) fill forEmptystars.numpy.maindexing bottleneck (506c2cf): ~38% fasterfit_motion_modelsby avoidingnumpy.ma's slow generic per-element indexing; reused one multiprocessing pool across the whole call instead of spawning one per motion-model group.StarTableconstruction (9287e51): gained an opt-incopy=False, and now builds all columns in a single constructor call instead of manyadd_column()calls (~1.2s / +4.6GB → ~0.001s / ~0GB for ~29 columns at 1.4M rows).vstackdouble-copy (6aa6d38): adding new-star rows built a whole parallel table andvstack-ed it onto the growingref_table, transiently holding old + new + concatenated data for every column at once — roughly doubling peak memory on every "add new stars" step. Now concatenates columns directly and drops old references immediately.fit_motion_modelsper-star overhead (9287e51, this session): eliminated an O(N_stars) data-prep cost that ran regardless of how few stars actually needed refitting (now sliced down first); removed a redundant double-copy incombine_lists/fit_motion_models's array prep; a per-star fixed-params dict is now built lazily only for stars whose motion model actually needs it (skipped for ~84% of stars — those already handled by the vectorized batch path — in one benchmark).match()gained aworkersparameter (default 1) forscipy.spatial.KDTree.query_ball_point;align.pyexposes it asmatch_workers(default 1, since production runs typically share a machine — opt in explicitly for the speedup). Verified this doesn't affect the delicatedm_min == dr_mintie-break, which depends on within-list neighbor order (workers=1vsworkers=-1give identical, order-preserved neighbor lists, checked against dense/duplicate-point edge cases and a full end-to-end run with all 37ref_tableoutput columns byte-identical). Also added a configurablemp_star_threshold(default 100,000): a multiprocessing pool for motion-model fitting is only spun up when the number of stars actually requiring the non-vectorized path meets this threshold, even ifprocesses > 1was requested — measured break-even for that fixed pool-spawn/IPC overhead was between 20,000 and 100,000 stars on a 10-core machine.Other
inherit_n_detect(14e7f00): newMosaicSelfRef/MosaicToRefparameter (defaultTrue) so a star'sn_detectreflects the total number of raw detections it represents across nested alignment layers, not just 1 per input starlist.tqdmprogress bar over a handful-of-motion-model-types loop (2d07f94).mm_reworkthat hadn't yet made it into this branch: apyproject.tomlfix (66ac298, originally4b2da35), adding the flystar modules to the Sphinx docs (7b3fb01, originally34611d4), and a docs formatting cleanup (a1b2409, originally92b44cd).Documentation
5f00827): added.readthedocs.yaml. Getting a working build also required fixing three real, pre-existing bugs unrelated to RTD specifically — they'd have broken a freshpip install .for anyone on a current Python:pyproject.toml's[build-system]pinnedcython==0.29.14(leftover astropy-template boilerplate for a package with zero.pyx/C extensions), which imports the stdlibcgimodule removed in Python 3.13, breaking install outright; the declareddependencieswere missingscipy,matplotlib,tqdm,joblib, andpandas, all imported unconditionally at module level; andsetup.cfg'sgithub_projectwas still the template default (astropy/astropy), whichconf.pyuses unconditionally to build doc issue-links. Verified via two independent fresh-venv builds (editable and the exact non-editablepip install .[docs]RTD runs) —sphinx-buildsucceeds, 92 warnings, all pre-existing docstring formatting nits unrelated to this change.wei-lingfeng/flystarrather than this repo directly — importingMovingUniverseLab/flystarinto Read the Docs needs repo-admin access (to add its webhook) that isn't available on this account. The fork tracks this branch 1:1, so content is identical; re-pointing Read the Docs at the org repo later (once an org owner grants access) needs no other changes.Validation
Each change was validated individually — synthetic fuzz tests against reference implementations (thousands to tens of thousands of cases per change), byte-for-bit output comparison on real multi-thousand and multi-million-star datasets, and dedicated new unit tests where behavior changed. See individual commit messages for specifics.
Full test suite passes except two pre-existing, environment/data-dependent failures unrelated to any change here:
test_masked_cols(missing test fixture file) andtest_generic_match(test data contains no finite values).