Skip to content

Follow-ups from the post-merge review of #299 (MIST evolutionary model) #305

Description

@jdeast

Post-merge review of #299 (MIST evolutionary-model component, merge 75c7d4e). The full write-up, with measurements and line references, is at https://claude.ai/artifact/NWrQHkEXjypSftHp9pAr3f. This issue carries the actionable list so the follow-up PRs have something to close.

Verdict: the core is sound and well argued, CI is green, the component tests pass, and examples/hat3/hat3_mist.yaml builds with a finite start logp and gradient and with the grid extents correctly applied as bounds. Two defects were measured, one design regression was found, and some merge debris rode in. Nothing breaks a fit.

cc @PhoebeSandhaus

Must fix

  • Undefined citations in every MIST paper draft. models/MIST/MISTv2.5/EEPs/MISTv2.5.grid.yaml says citation: "Dotter:2026, Bauer:2026, Dotter2016, Choi2016"; the bib has Dotter:2016 and Choi:2016 (with colons). MISTv1.2/BCs/MISTv1.2.grid.yaml has the same two broken keys. The string is spliced raw into \citep{} at runtime so the bib cross-reference test cannot see it, and the synthetic-grid fixture writes the correct keys so its round-trip test cannot fail. Same prose paragraph (evolutionarymodel.py:916-918): $M_\\odot$ inside an r-string emits a literal \\odot, and there is no period between "10 M_sun" and "Thus".
  • The EEP seed searches the wrong track (_seed_eep_hint, stage 1). It reads the star.logmass initval before the relaxation engine derives it from a user star.mass (stage 4), so it walks the defaults.yaml 1.0 solMass track. Measured on hat3_mist (user mass 0.92): seeded EEP 348 with seed chi2 19.4 on the true track, versus 338 / 0.16 when the right track is used; the age term alone pays 29 nats at the start. Every shipped params file seeds mass, not logmass. PR warn when the model does not start where the user asked #252's check_user_starts will not flag this because eep is not user-set; its lesson (only sampled leaves reach the model, so back-solve the leaf) is the fix: resolve star.mass too and use log10 of it when logmass is at its default. A general post-finalize hint pass is a separate notes item if a second component ever needs it.
  • The GUI lost the Kiel diagram. Commit 9b3cb99 deleted EvolutionaryModel.plot_data and the plot_via_specs one-liner in favour of the hand-drawn MISTPlot; the base plot_data returns [], so gui/tune.py and evaluator.py get nothing. evolutionarymodel.md's Plots bullets, the comment above compile_plotters, and the docstring at tests/test_evolutionary_model.py:1256 still describe the shared-Chart design. MISTPlot._kiel_render_spec_groups is a copy of plotrender.render_spec_groups differing only in legend de-duplication, which belongs upstream in both renderers. No test calls comp.plot or comp.plot_data.
  • Multi-star plotting is broken (latent in the single-star example). Parameter.summary is a list for vectors longer than 1 but MISTPlot treats it as one object: posterior error bars go silently to zero and plot_contours raises AttributeError, aborting the per-star loop. plot.py:267 indexes star names by instance index rather than through star_indices. The contour path ignores points, so per-mode contours draw the combined posterior, and finally: plt.close(fig) raises UnboundLocalError when the KDE fails, masking the real exception.
  • Merge debris in sed.py. SED._add_prose (sed.py:1708-1776) is never called and contains a "DEAL WITH CITING THIS LATER" placeholder, a copy-pasted EEP-Jacobian paragraph that is false for an SED, and a self.star_indices the SED does not have. Its only input, an unconditional open() of the BC grid yaml in load_data (sed.py:603-608), now runs on every SED fit for nothing. Wire and correct it, or drop both. The SED palette and marker-edge changes are unrelated cosmetics; nothing broke.

Small cleanups

  • _seed_eep_hint reads MISTPlot.KIEL_EEP_WINDOW[0] for its pre-MS steer; give the seed its own named ZAMS constant so a plot-window change cannot move a start.
  • outputs/contour_plot.py reimplements 2-D posterior contours that corner_utils already gets from corner.hist2d, with MIST-specific defaults in a generic module; build on corner or make it a Chart, and document it. Remove the @staticmethod decorators on the module-level functions in outputs/plot_helper_functions.py.
  • load_data reads the grid yaml by hand (import yaml inside the loop, hand-built path, self.model/self._model_yaml overwritten per instance); route through mist_grid.grid_path.

Design decisions on record (no action beyond documentation)

  • normalize=True on all four penalties is a deliberate departure from massradius_mist.pro, and the right one: the chi2-only form leaves an implicit sigma_R sigma_T sigma_age prior after marginalizing the fitted star quantities.
  • No age ceiling (JDE, 2026-09-17): fine, and better than EXOFASTv2's hard 13.82 Gyr cutoff; the star.age bound plus the floored tie do the work, and the floor exists precisely to allow for model systematics. No extra penalty on the predicted age. Record it in evolutionarymodel.md.
  • Mass loss assumed zero (JDE, 2026-09-17): intentional deferral, fine. When the grid grows a current-mass column, EXOFASTv2's >1% warning is the model.
  • constrain: is ignored by structure_consumers, as the PR body says; one decision across mann/torres/evolutionarymodel.

Test gaps

  • _track_table depends on feh and EEP only, so the two mass-neighbour synthetic tracks are identical: the clamp-not-extrapolate test, the interpolation tests and the dense-array index-order test all pass with a reversed mass axis. Add a mass term to one column.
  • The Jacobian's two-sided clip is tested against a test-local copy of the formula, not the model's eep_age_jacobian potential; reverting the model to a bare floor leaves the tests green. Nothing evaluates a *_prior potential's value, so normalize=True and the absolute-dex feh sigma are unpinned. Pin both at the start point on a synthetic grid carrying a 2**32 and a near-zero dEEP_dage row.
  • hat3_mist.yaml never prepares in CI (grid absent, so test_shipped_example_prepares skips forever). Monkeypatch DEFAULT_MIST_MODEL_ROOT to the synthetic root when the real grid is missing, or give it its own prepare test.
  • test_an_unpublished_model_release_raises patches DEFAULT_MODEL_ROOT instead of DEFAULT_MIST_MODEL_ROOT, so it exercises the explicit-root branch, not the one it names.
  • The premature-block warning has only a negative test; restore a positive one for an unbacked block. No test asserts which EEP the seed picks; test_track_parameters_resolve_to_their_defaults's docstring claim about eep is false once the hint lands.
  • Untested: user *_floor: and dragon_penalty_weight: keys; a user entry freeing _pin_unmodeled_stars; the mist: True, parsec: True warning via System.prepare(); _warn_outside_grid; load_mist_grid end to end on a grid with holes, duplicate ages or a 30.0 row. write_synthetic_mist_grid is a third copy of the v2.5 filename rule; call mist_grid.grid_path.

Suggested grouping

  • A. Paper output: citation keys, \\odot, dead SED._add_prose, fixture reads the shipped citation.
  • B. EEP seed: back-solve logmass from a user mass, own ZAMS constant, pin the chosen EEP.
  • C. Plotting: restore plot_data, legend de-dup upstream, multi-star fixes, contours on corner, two-star test.
  • D. Tests and docs: mass-dependent synthetic column, pin the Jacobian and one prior value, hat3_mist in CI, repair the two tests, record the two rulings.

🤖 Generated with Claude Code

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions