Add globular_cluster() template + 20-year cluster animation scripts#181
Add globular_cluster() template + 20-year cluster animation scripts#181astronomyk wants to merge 7 commits into
globular_cluster() template + 20-year cluster animation scripts#181Conversation
astronomyk
added
enhancement
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
==========================================
- Coverage 77.09% 75.08% -2.02%
==========================================
Files 34 37 +3
Lines 1698 1870 +172
==========================================
+ Hits 1309 1404 +95
- Misses 389 466 +77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
How can I stop you from slapping the |
|
This is, as you mentioned, based on #180, thus includes all the changes from there. I will review it once you merged #180 and then rebased this PR. To do so, please DO NOT just click on the |
|
Also, may I kindly remind you that the PR titles are used to build the release notes and serve as an integral part of the documentation. Please change the title here to something that reflects that (as in e.g. #180). |
|
Keeping the bastard honest. Thanks for the tips ;) I'll close this PR as I've asked claude what it would change based on your comment to #180 . If it decides on some refactoring, I'll update this branch based on the changes in the other one. |
|
Also, fwiw, these are here just to get some options on the table for next week. Don't worry about the Salpeter code - targets is still what I'll be pushing in Dwingeloo. If anything, new stuff here should be seen as potential inspiration for "real" implementations in targets |
Builds a fictional Omega-Cen-like cluster: Salpeter IMF for masses, mamajek mass->spectral-type and mass->Mv mapping, uniform random spatial distribution within a square FOV, and a randomly drawn closed orbit around a central intermediate-mass black hole that RV-shifts each star's spectrum. Reuses the Kepler/Thiele-Innes helpers from _gillessen.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
astronomyk
force-pushed
the
kl/globular_cluster_imbh
branch
from
ced3340 to
ed31095
Compare
Adds two top-level scripts (plot_galactic_centre_cluster.py, plot_globular_cluster.py) that render 20-year evolution GIFs from each template's scopesim.Source, one frame per Source built per epoch. Functional changes to enable the animations: - galactic_centre: expose rv column in the fields table. - globular_cluster: add a `time` parameter (years); positions are now the orbital projection of randomly drawn closed orbits at `time` rather than uniform random within the FOV, so both positions and RVs evolve coherently. Default outer semi-major axis tightened to 0.25 * fov * distance_pc to keep most stars inside the FOV. Add rv column to the fields table. Tests updated and added: rv-column presence in both templates, time-advance state evolution for globular, position-bounds test relaxed for orbital projection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…view Changes plot_globular_cluster.py defaults to density=750, fov=2.0 (=> 3000 stars), imbh_mass=1e5, axis_limit_arcsec=1.0. Marker sizes shrunk and edge stroke removed so the dense field stays legible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Relocates plot_galactic_centre_cluster.py, plot_globular_cluster.py and their rendered GIFs from the repo root to scopesim_templates/tests/visual_inspection/. Each script's default output path is now anchored to its own location (Path(__file__).parent), so the GIF lands next to the script regardless of the caller's CWD. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The strip-and-reattach round-trip for the rv column was introduced when the rv column was added on the animation branch. Now that cluster_plots has fast-forwarded into globular_cluster_imbh, fix it here: - galactic_centre.py and globular_cluster.py hold rv as a Quantity from the moment it is computed (or pulled from the gillessen QTable) through to the final fields-table column. Only stripped to a plain float at the per-star Spextrum.redshift(vel=...) call site. SpT mapping refresh on galactic_centre.py: switched the if/elif/else to a dict.get() lookup; updated warning wording to "missing or unrecognised SpT" (the Gillessen catalogue legitimately has empty SpT cells for S39 and S55). Test regex updated to match. YAGNI cleanup from the original review (drop library/catalogue/R0 params) is already in main via PR #180; not re-applied here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Righto. Rip into it @teutoburg |
astronomyk
changed the title
teutoburg
left a comment
teutoburg
left a comment
There was a problem hiding this comment.
The "request changes" is mainly about the two float(rv[i].value) cases, which are actually wrong because .redshift() assumes m/s, see the individual comment.
Hint: Remember you can "Add suggestion to batch" in the Files changed tab to commit all my suggestions at once (or twice, if splitting corrections and stylistic changes).
One note on the "visual inspection" tests: I like that these exist in principle, but we usually put things like that in the docs. Doesn't have to be a notebook, but I think they are at risk of getting lost in the tests here. Tests are usually all that runs automatically, without requiring visual inspection, whereas the RTD build is the thing that we usually (should) check manually to confirm outputs are as expected.
teutoburg
changed the title
teutoburg
changed the title
globular_cluster() template + 20-year cluster animation scripts
PR #181 review (teutoburg) caught that both galactic_centre() and globular_cluster() were passing redshift velocities as bare floats: base[spec_types[i]].redshift(vel=float(rv[i].value)) Spextrum.redshift(vel=...) assumes m/s for a bare number, so the catalogue's km/s values were silently under-applied by 1000x. The rv-shift tests still passed because they only checked for *any* flux difference between two stars, not the size of the shift. Pass the Quantity through instead - redshift() handles it natively: base[spec_types[i]].redshift(vel=rv[i]) Also from the same review: - Drop the `mass` column from globular_cluster's fields Table - scopesim ignores extra columns and it was only a debug breadcrumb. Tests that previously read field["mass"] now sample masses directly via _sample_salpeter_masses, or use spec_types/weight as the seed-invariant assertion target. - Drop the redundant int() wrapping round() (round returns int). - Fix docstring backtick style: `name` not ``name`` per numpy guide. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #181 review (teutoburg) preferred that the cluster GIFs and their producing scripts live with the docs, not under tests/. The animations are demos, not test assertions - tests/ runs in CI without visual review, whereas docs/notebooks/ feeds RTD where the GIFs can be seen. - Move plot_galactic_centre_cluster.py -> docs/notebooks/plot_galactic_centre.py - Move plot_globular_cluster.py -> docs/notebooks/plot_globular_cluster.py - Move both GIFs to docs/notebooks/ next to their scripts. - Delete the now-empty scopesim_templates/tests/visual_inspection/. - Add two MyST notebooks (galactic_centre.md, globular_cluster.md) with a brief description, a quick `src.fields[0].field` example, and the pre-rendered GIF embedded via `:figure:`. The GIFs are not regenerated on doc-build (matplotlib FuncAnimation is too slow); contributors re-run the scripts manually to refresh them. - Wire the two new notebooks into docs/index.md toctree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| ... distance_modulus=13.6, seed=42) | ||
| """ | ||
| # 1. N stars from density and FOV | ||
| n_stars = round(float(density) * float(fov) ** 2) |
There was a problem hiding this comment.
It was perhaps less obvious than I thought from my original comment that the thing to keep here was int, not round, because int also rounds (or rather floors, but irrelevant for this case), but round still returns a float, which isn't really right for a number of stars. But if the float can be digested without errors further down, it doesn't really matter...
2 participants
Summary
stellar.globular_cluster()— a Source builder for a fictional Omega-Cen-like cluster with a central intermediate-mass black hole. Stars are drawn from a Salpeter IMF; mamajekmass2spt/mass2absmagprovide per-star spectral type and absolute V mag; each star is placed on a randomly drawn closed Keplerian orbit around the IMBH. The line-of-sight velocity at the user-suppliedtimeRV-shifts the star's spectrum.rvcolumn to theSource.fieldstable on bothgalactic_centre()andglobular_cluster(), and atimeparameter onglobular_cluster(). Together these expose enough per-star state through the Source API alone to drive a time-evolution animation.scopesim_templates/tests/visual_inspection/:plot_galactic_centre_cluster.py— 20-year GIF of the Gillessen+2017 S-star field around Sgr A*.plot_globular_cluster.py— 20-year GIF of a 3000-star, 1e5 M☉ IMBH cluster at ±1″.Each frame is built from a fresh
scopesim.Source, so the animations end-to-end test the function and the Source object.rvarrays are held asastropy.units.Quantityend-to-end (no more strip-and-reattach round-trip with the fields table); SpT mapping ingalactic_centre()switches to adict.get()lookup and the warning wording is now "missing or unrecognised SpT" (the Gillessen catalogue legitimately has empty cells for S39 and S55).Test plan
python -m pytest scopesim_templates/tests/test_stellar -m "not webtest"passespython -m pytest scopesim_templates/tests/test_stellarpasses (the cluster tests are marked webtest because they hit pyckles + SVO filter curves)python scopesim_templates/tests/visual_inspection/plot_galactic_centre_cluster.pyproducesgalactic_centre_evolution.gifnext to the script; S2 should sweep visibly past Sgr A* mid-windowpython scopesim_templates/tests/visual_inspection/plot_globular_cluster.pyproducesglobular_cluster_evolution.gifnext to the script; inner stars should complete visible loops over 20 yr while outer stars barely move🤖 Generated with Claude Code