Modernise, test and fix the CME codebase - #1
Merged
Conversation
The README described the project as ready to use ("Not a single line of
code is required by the user") while omitting that it cannot run: the
`modules` I/O package it depends on is absent from the repository, the
code is Python 2 only, and it targets the `enthought.*` Traits namespace
retired in 2011.
Rewrite it to be accurate and useful:
- state the archived status and the Python 2 / ETS 3.x constraint up front
- document the missing `modules` dependency and what each import provides
- list the dependency set reverse-engineered from the imports, since there
is no manifest
- record the run-directory naming convention, which is encoded implicitly
in 18 places in the source and nowhere else
- fix the broken screenshots/inspectparams.png link and surface the four
screenshots that were present but unreferenced
- add MUSIC / Gadget / Rockstar / consistent-trees citations
Add AUDIT.md recording the findings behind that rewrite: the blocking
issues, 18 correctness defects with line references, security and
robustness concerns, the duplication inventory, and a prioritised plan.
Nothing was ignored previously, so Python caches sat alongside generated simulation artifacts as untracked noise. The Gadget tab writes its driver script (rungadget.sh) into the working directory and MUSIC leaves noise and scratch files behind, all of which are reproducible from GUI state and belong with the run directory rather than in version control.
The repository had no license file, leaving it all-rights-reserved by default while the README invited others to use it for their own simulation suites. MIT is the conventional choice for tooling of this kind.
Four of the eleven modules did not parse under Python 3 at all, so the package could not be imported on any currently supported interpreter. - convert 35 `print` statements to function calls, preserving the space-separated multi-argument output - replace `xrange` with `range` (6 sites) - replace the pandas `.ix` accessor with `.loc` (21 sites). `.ix` was removed in pandas 1.0; these indexers are all label-based lookups of Rockstar halo IDs against the catalogue index, so `.loc` is the correct replacement rather than `.iloc` - escape five stray backslashes in ics.py print literals that Python 3.12 reports as invalid escape sequences, keeping the rendered output identical Verified: every module now compiles cleanly with no SyntaxWarnings. Behaviour is otherwise unchanged; this commit is mechanical.
The `enthought.*` namespace packages were dropped in ETS 4 (2011), so `enthought.traits.api` and friends do not exist in any installable release. Map them onto their modern homes: enthought.traits.api -> traits.api enthought.traits.ui.api -> traitsui.api enthought.traits.ui.wx.editor -> traitsui.wx.editor enthought.traits.ui.wx.basic_editor_factory-> traitsui.basic_editor_factory enthought.enable.api -> enable.api wxPython remains the toolkit, so this is an import-path change only. Also replace the four star imports with explicit names and declare `__all__`, since `from Common import *` in every tab previously re-exported the entire transitive import graph. Two concrete problems this fixes: - ics.py uses `patches.Rectangle` without importing it. That resolved only because `from matplotlib import *` happened to run after pyplot had been imported, which populates `matplotlib.patches` as a side effect. Reordering two lines in Common.py would have broken the ICs tab. It is now imported explicitly. - `Figure` now comes from `matplotlib.figure` rather than `matplotlib.pyplot`, so the module no longer pulls in pyplot's global figure registry, which should not be involved in an embedded canvas. Dropped 11 names that were imported but never used (TableEditor, EnumEditor, RangeEditor, Handler, Label, NoButtons, Spring, spring, UItem, TabularEditor, TabularAdapter) plus unused sys, socket, pylab and randint imports. Verified by AST analysis that every free name in all nine tab modules is still supplied; the only unresolved name is the pre-existing dead `Analysis` reference in main.py, removed in a later commit.
The eleven modules sat flat in the repository root, importable only by running from that directory, with no packaging metadata. Move them into a src layout and give the tabs names that describe what they are: main.py -> src/cme/app.py Common.py -> src/cme/ui/common.py header.py -> src/cme/ui/home.py (it is the Home tab) gadgetrun.py -> src/cme/ui/gadget.py contam.py -> src/cme/ui/contamination.py ics.py, halos.py, candidates.py, install.py, mergertree.py -> src/cme/ui/ This commit is moves plus the import rewrites they force, so the diff is reviewable as a rename. `src/cme/ui/` is where the remaining display-coupled code lives; subsequent commits pull the computation and file-format writing out into sibling packages that can be tested without a display. Also add pyproject.toml, since the dependency set previously had to be reverse-engineered from imports. Note the split: the base dependency set is numpy and pandas only, because the extracted modules that follow are display-free and must stay testable in CI without a GUI toolkit. Matplotlib, traits, traitsui, enable, mayavi and wxPython move to a `gui` extra. Adds a `cme` console script entry point, replacing `python main.py`.
The run-directory convention H<halo>_B<region>_Z<zinit>_P<pad>_LN<lmin>_LX<lmax>_O<overlap>_NV<nrvir> was rebuilt inline in 18 places, and the Lagrangian region paths in 7 more. Nothing recorded the convention in one place, so the copies had drifted. Replace them all with cme/naming.py, which is display-free and unit tested. Three bugs were a direct consequence of the duplication, and are fixed by routing every caller through the shared helpers: 1. Snapshot indices were zero-padded by concatenation: "groups_0" + str(n). That is only correct for two-digit snapshot numbers. Snapshot 5 produced "groups_05" where Gadget writes "groups_005", so every existence check against a single-digit snapshot reported "not found"; the zoom tabs default to snapshot 255, which produced "snapdir_0255". Now formatted to a fixed width of 3. 2. The contamination and halo tabs hardcoded "_Z127" rather than reading their own zinit trait, so any run generated at a different starting redshift was looked up in a directory the ICs tab had never created. zinit is now a parameter that defaults to 127. 3. Base paths were concatenated as `self.gadpath + 'halos/H'`, which silently produced ".../datahalos/H190897" whenever the path trait had no trailing slash. Now joined with os.path.join. Also replaces the hardcoded snapshot 63 in the Gadget existence check with the last snapshot implied by the requested output count, so the table is correct for output counts other than 64. The contamination tab shrinks by 50 lines: eight of its parameter handlers were byte-identical 8-line blocks, now one `_refresh()` call each. That collapse also fixes AUDIT 2.1.3, where four of those handlers wrote `self.makeactive == True` instead of `=`, leaving the plot buttons permanently disabled after changing overlap or snapshot number. Two further defects were identified while extracting this and are fixed in later commits, to keep this one mechanical: the ICs tab iterates over its `lmin` Enum with `for lmini in self.lmin`, which iterates the characters of the string and so breaks for levels >= 10; and the Lagrangian display traits format a List with str(), producing "NRVIR['1']". 23 tests, covering the format, the round-trip parse, the type coercion the GUI forces (strings from CheckListEditor, ints from Range), and a regression test per bug above.
The Hubble parameter appeared as six hardcoded 0.6711 literals across four tabs, and the full parameter sets came from `grifflib.cosmoconstant` in the external `modules` package that is not distributed with this repository. A value duplicated six times is a correctness risk in a codebase whose output is scientific, and depending on an absent module for it made the values unauditable. cme/cosmology.py holds one frozen dataclass per set (WMAP1/3/5/7/9, PLANCK) with a literature citation on each, and a `cosmoconstant()` shim preserving the old six-tuple signature so call sites are unchanged in shape. On the values: `grifflib` is absent, so these were taken from the cited papers rather than copied from the original. The PLANCK row cross-checks exactly against the values hardcoded as defaults in the Gadget tab (Omega0 = 0.3175, OmegaLambda = 0.6825, h = 0.6711), and its Omega_b agrees with Planck 2013's Omega_b h^2 = 0.02205 to three decimal places, which is good evidence the conventions match. This is called out in the module docstring: if the original grifflib is recovered, the non-PLANCK rows should be diffed against it before being trusted for new science. Both `h` and `H0` are exposed because the two output formats disagree -- MUSIC configs want H0 in km/s/Mpc, Gadget parameter files want h -- and the old tuple carried H0 while the Gadget tab's own default carried h. Tests pin that convention so the ambiguity cannot silently flip; the resulting double-conversion in the Gadget writer is addressed in the next commit. Tests assert flatness, physical plausibility and a citation for every set.
MUSIC configs, Gadget parameter files, Config.sh and the batch submission
scripts were all built by long runs of sequential f.write calls interleaved
with GUI state -- roughly 110 writes for param.txt, 90 for Config.sh, 50 per
MUSIC conf. None of it could be tested, so a malformed parameter file would
only surface once it had consumed a cluster allocation.
cme/writers/ holds pure functions that take scalars and return text:
music.py resim_conf, parent_conf, refinement_seeds
gadget.py param_file, config_sh, expansion_list, softenings, pmgrid_for_levelmax
submit.py slurm_script, pbs_script, mpirun_command
Tests parse the output back with configparser (MUSIC) and a key/value
splitter (Gadget) rather than only string-matching, plus a byte-exact golden
file for a MUSIC resim conf.
Fixes carried by the extraction:
- AUDIT 2.1.1: SLURM submission raised TypeError, concatenating integer traits
with strings ('#SBATCH -n ' + self.SLURMcores). SLURM had never worked.
- AUDIT 2.2.5: the SLURM mpirun line was built from the *PBS* core count, so a
SLURM job would have launched with the wrong number of ranks.
- AUDIT 2.2.6: both branches of _SLURMcores_changed were identical, so
enabling SUBFIND left SLURM pointing at the plain binary and param.txt while
PBS correctly switched to P-Gadget3_sub and param_sub.txt.
- AUDIT 2.1.11: determineboolstr returned an unbound local for anything not
exactly True or False (a numpy bool, say). Replaced by a total function.
- AUDIT 2.1.12: the run script was opened before its directory was known to
exist; writers.write now creates parents and is called once the path is known.
- HubbleParam was stored as H0 in a trait named for h and divided by 100 when
written, so the value displayed in the GUI disagreed with the value on disk.
The writer takes the cosmology and emits h directly.
- Config.sh emitted `TOKEN# comment` with no separating space for tokens longer
than the comment column, e.g. FOF_SECONDARY_LINK_TYPES=4+8+16+32. Found by a
test asserting the separation, now guaranteed.
Deduplication: the 6 handlers that rebuilt the mpirun line held 18 copies of
it between them and are now one execute_command(); the 12-line softening
cascade appeared twice and the PMGRID thresholds once, all now in the writer.
ics.py loses 124 lines, gadget.py 215.
Whitespace differs from the old output in one respect: seed[10] and above were
previously misaligned by one column because the padding was a fixed string.
MUSIC's parser is whitespace-insensitive, so this is cosmetic.
The isolation-criteria scan -- the scientific heart of the tool -- lived inside a button handler, interleaved with figure clearing and axis labelling, and could not be exercised without a display and a real parent catalogue. It is now a pure function over a host catalogue with 30 tests against synthetic inputs with known answers. Fixes: - AUDIT 2.2.3: the results array was seeded with np.zeros((1, 6)) and grown by vstack, so every selection carried a phantom zero-mass candidate at the origin. It was plotted, included in the array view, and -- because the "add halo" guard tested `len(...) != 1` to detect the sentinel -- could be added to the working sample as halo ID 0. select() now returns an empty array when nothing qualifies, and the guard tests for no rows. - AUDIT 2.1.8: _clear_button_fired assigned to `self.main.mergertreetab.initstab`, a stray attribute on the wrong object, so clearing the sample left the ICs tab holding the old one. It also never cleared the contamination or Gadget tabs. Both paths now go through one _publish_sample(), which was also duplicated (the merger tree tab was assigned twice in _addhalo_button_fired and the contamination tab omitted). - AUDIT 4.5: the six position and mass arrays were re-extracted from the catalogue inside the per-candidate loop, making the scan O(N^2) in the base sample, and each accepted candidate triggered a full-array vstack copy. Extraction now happens once and results accumulate in a list. A 4000-halo sample completes in under a second. The exclusion zones are now first-class: an ExclusionZone is either an absolute mass threshold or a multiple of the candidate's own mass, and the relative form excludes the candidate from its own comparison (a halo is zero distance from itself). The GUI supported both per zone but the code only ever applied a relative threshold to zone 1 and absolute ones to zones 2 and 3. Tests cover the hubble conversion on both mass and distance, three-dimensional separation, per-candidate threshold scaling, the default Caterpillar criteria as an integration case, and the candidates.dat round trip.
The Lagrangian region builder re-read four large blocks from the parent simulation on every iteration of the nrvir loop, none of which depend on nrvir: the snapshot positions and IDs, and the IC positions and IDs. For a 512^3 parent each POS block is roughly 1.6 GB, so a sweep over five region sizes moved about 30 GB from disk to compute the same four arrays five times. Read them once before the loop. The particle radii from the candidate centre are invariant too -- only the acceptance threshold moves with nrvir -- so R is computed once as well, and the position block it derives from is freed immediately afterwards. Peak memory is no worse. The original held the (N,3) position block alongside three (N,) difference arrays and R; this holds the (N,3) IC position block alongside three (N,) index and ID arrays, and it never materialises dx/dy/dz as separate arrays. Also drops `currentpos = snapPOS[Rindex[0]]`, a large allocation whose result was never read, and replaces three copies of the hardcoded parent snapshot path with module constants noting that they belong in configuration.
Traits accepts assignment to an undeclared attribute by creating it, so a misspelled trait name fails silently: the real trait keeps its default and the feature it controls quietly does nothing. Four instances: - AUDIT 2.1.2, gadget.py: `self.nvir = ['3',...]` where the trait is `nrvir`. nrvir stayed an empty list, so every `for nrviri in self.nrvir` loop body was skipped and the Gadget parameter sweep produced nothing at all. This is the most consequential of the four -- the tab's main action was inert. - AUDIT 2.1.7, home.py: `self.clustopt = 'odyssey'` where the trait is `clusteropt`, so the Harvard Odyssey preset never applied and the cluster stayed at its default. - AUDIT 2.1.9, ics.py: `self.pre_smooth` / `self.post_smooth` / `self.grad_order` where the traits are `presmooth` / `postsmooth` / `gradorder`. Three MUSIC Poisson-solver settings were never applied. Compounding it, `presmooth` and `postsmooth` were declared as plain class integers rather than traits, while the view built Item()s for them; they are now Int traits. Fixing the grad_order typo made an existing correct assignment redundant, so the duplicate is removed.
Seven defects across the halo and ICs tabs. Wrong results: - AUDIT 2.2.1, halos.py: the 3D halo distribution assigned `z = tmphalos['posY']`, so every full-box Mayavi scene was flattened into a plane with the y coordinate plotted twice. - AUDIT 2.2.2, halos.py: `self.hostposx = host['posZ']`, so the reported host x position was actually its z position. - AUDIT 2.2.4, ics.py: the displayed Lagrangian pointer name and path formatted the nrvir CheckListEditor list with str(), producing "NRVIR['1']" -- visible in screenshots/constructICs.png -- while the files were written with int(nrvir[0]). Displayed and actual paths disagreed. Both now go through naming.lagr_region_path, and the labels clear when the selection is ambiguous. - ics.py: `for lmini in self.lmin` iterated an Enum holding a single string, so a levelmin of 10 or above walked its characters and ran the sweep twice with levelmin '1' and then '0'. lmin is not a list like the other sweep axes. Crashes and dead code paths: - AUDIT 2.1.4, halos.py: gethostid() returned an unbound local when no row matched, raising UnboundLocalError, even though every caller tests the result for truthiness as though a miss were expected. It now returns None, tolerates a missing summary file, and skips short rows. - AUDIT 2.1.5, halos.py: gethalos_xy() read `idhost` in its radius block, but only the zoom branch defines it, so every parent full-box plot with two position axes raised NameError. It also returned unbound `x, y` when the catalogue was missing. The method is restructured with the catalogue load and halo-type selection pulled out, and returns empty arrays on a miss. - AUDIT 2.1.6, halos.py: the entire plotting block in _plot_button_fired sat inside the `elif parentorzoom == 'zoom'` branch, so pressing Plot in parent mode computed a path and then fell off the end of the method drawing nothing. - AUDIT 2.1.10, ics.py: `nhalo` was unbound when the selected halo was absent from the candidate file. Replaced with a vectorised lookup that reports the miss through the status field. Also replaces the hardcoded /bigbang absolute path in gethostid() with a summarypath trait derived from the project data directory, and renames the `reWriteIC` import from `re` -- which shadowed the standard library module -- to `rewriteic`.
Every external invocation was assembled by string concatenation and handed to os.system or subprocess.call(..., shell=True) with paths from Directory traits interpolated in -- 70 call sites. Two consequences that bite in practice on a shared filesystem: - a path containing a space silently splits, so /data/My Runs/H190897 arrives as two arguments; - a path containing ;, $(...) or && executes. All are legal in a directory name. Add cme/shell.py, whose run() takes an argument list and an explicit cwd, and convert the callers: - install.py: 60 os.system calls and 24 os.chdir/chdir-back pairs become a table of source trees and configure flags plus one _build() method. The file drops from 205 lines to 132. The chdir pairs were not exception-safe, so a failed build left the process in the source tree. run_all() also stops at the first failure, where the original `;`-joined strings ran `make install` even after `configure` had failed. Progress now goes to a status field rather than print(), which a GUI user is not necessarily watching. - ics.py: `cd <dir>; <music> <conf>; rm wnoise* temp*` becomes run_music(), which runs MUSIC with cwd set and only cleans up on success. Because the original parts were `;`-joined rather than `&&`-joined, the rm ran even when MUSIC had failed. Scratch removal now globs within one directory instead of shelling out to rm. - gadget.py: the file operations are done in Python. param.txt is written straight into the run directory instead of into the working directory and then `mv`ed; `mkdir -p` becomes os.makedirs(exist_ok=True); and the `tail -n+96 | cat >> | cp` pipeline that splices the generated Config.sh onto the upstream tail becomes install_config_sh(), which needs no temporary file and reports a missing upstream Config.sh instead of silently producing an empty splice. The generated cluster driver script still exists -- it does module loads, ssh and qsub on a remote host, which is genuinely shell work -- but is now invoked as ["bash", path] rather than through a shell, and its name is a constant rather than a literal in two places. No `shell=True` or `os.system` remains in the codebase.
The Home tab identified the machine with an if-chain on platform.node() and
then assigned hardcoded absolute paths, so the tool only worked for its
original author on five named machines. Anyone else had to edit the source. It
carried 19 personal identifiers between home.py and gadget.py: usernames
(bgriffen, uqbgriff), hostnames (csr-dyn-150.mit.edu, bigbang.mit.edu,
rclogin13.rc.fas.harvard.edu, Brendans-MacBook-Pro.local), absolute paths
(/bigbang/data, /n/home01/bgriffen/data, /Users/griffen/Desktop/cme) and two
email addresses.
clusters.toml describes each machine as data: which hostnames select it, where
home directories live, and where MUSIC, P-Gadget3 and the project data sit,
using {home}/{user}/{master} placeholders. cme/config.py loads it from
$CME_CLUSTERS, then ~/.config/cme/clusters.toml, then the repository copy, and
falls back to a single local profile so the application still starts on a
machine with no configuration and no TOML parser.
The two path derivations are now one. Previously _clusteropt_changed and
_username_changed computed *different* path sets from the same inputs -- the
latter derived parentsimpath as {master}/AnnaGroup/caterpillar/parent/512Parent/
while the hostname branches used {home}/AnnaGroup/caterpillar/parent -- so which
paths you ended up with depended on the order you touched the fields. Both
handlers now call one _apply_profile().
The Gadget tab picks up its scheduler, queue and extra directives from the
profile, and takes the Gadget source tree from the Home tab instead of two
hardcoded absolute paths selected by scheduler type. Its `ssh antares` hop and
the `unloadmods`/`loadgadget` lines become configurable rather than baked in;
the matching `logout` that closed the ssh heredoc goes with it. SLURMqueue was
an Enum of three Harvard queue names, which could not hold the queue another
site's profile specifies, and is now a Str.
24 tests, including that every hostname the old if-chain handled still resolves
to the right profile, that no placeholder survives resolution, and that no
resolved path contains a personal home directory.
Dead code (AUDIT 2.3 and beyond): - app.py: `_analysistab_default` instantiated `Analysis`, whose import was commented out and for which no trait exists -- a latent NameError. Removed along with the commented-out import and two commented Instance declarations. - app.py: ApplicationMain.__init__ never called HasTraits.__init__, so trait initialisation was skipped and kwargs were silently discarded. - halos.py: `boxtype` was declared twice, the second silently winning. - Three unused `modules.mergertrees` imports (only the merger tree tab uses it), five unused Button traits wired to no handler and no view, `PBSstring` and `PBSjobname`, and ~40 lines of commented-out sample lists and debug prints. - 20 dead local assignments across four tabs, several of which existed only to feed commented-out debug prints. Removing `xpossub`/`mhalfsub` and friends in the contamination tab cascaded: the sub-halo sort they fed was itself unread. - The commented-out PBS example string embedding a personal absolute path. Duplication: - mergertree.py: four byte-identical `_*_changed` handlers become one `_invalidate_plot`, each of which also fetched an unused `ax`. File handling: - No raw `open()` remains outside cme.writers. The Gadget driver script is accumulated in a list and written once rather than held open across the whole sweep, and the two Lagrangian output files move to writers.lagrangian_header / writers.lagrangian_region. - Two `try: open(path)` existence checks become os.path.exists, and the one that reported failure via print() now sets the status field. Style: 17 `== True` / `== False` comparisons simplified (leaving the `enabled_when` strings alone -- those are Traits expressions, not Python), tabs in install.py converted to spaces, and printf-style formatting modernised. CI runs pytest, doctests, ruff check and a format check on three Python versions, plus compileall over the GUI modules -- which cannot be imported without a display or the absent `modules` package, but must at least parse. `ruff check .` is clean; the format check is scoped to the display-free modules and tests, since reflowing the TraitsUI view declarations would bury this branch's behavioural changes under whitespace.
The README still described a Python 2 package with no manifest, no tests and no license, run as `python main.py` from a flat directory. Bring it in line: - Python 3.9+, MIT, 171 tests in the badges - the status callout now distinguishes what works from what does not: the display-free library is pip-installable and tested, while the GUI still cannot start from a clean checkout because the `modules` I/O package is not distributed here - installation via the base / gui / dev extras, and `cme` as the entry point - a Library section showing the extracted API, since that is the part a reader can actually use - a Cluster profiles section documenting clusters.toml and its lookup order - the project structure and testing sections rewritten for the new layout - Known limitations replaced: the old entries were the defects this branch fixed. The honest remaining ones are the absent `modules` package, the complete lack of GUI test coverage, wxPython being legacy, and two parent paths still hardcoded in the ICs tab. AUDIT.md gains a Resolution status section: a table mapping each original finding to how it was addressed, a list of the twelve further defects found while doing the work, and eight outstanding items with the reasons they were deferred. The findings themselves are kept in full -- they are the rationale for the changes and the reference the regression tests cite. Also corrects one count in the duplication inventory: the mpirun execute-line builder had 18 copies across 6 handlers, not 12. The original figure counted lines rather than occurrences.
Two leftovers from the cosmology extraction, both concerning the meaning of HubbleParam. The tab's __init__ still carried Omega0, OmegaLambda and HubbleParam as literals, and _subscript_button_fired unpacked cosmoconstant() positionally -- which puts H0 (67.11) into HubbleParam. Now that the parameter file writer takes a Cosmology and emits the dimensionless h, that assignment left the displayed value disagreeing with the written one by a factor of 100, in the opposite direction to the original bug. Both now read the fields off the Cosmology object, so the table in cme.cosmology is the only place these numbers appear.
Points every contact reference at contact+cme@grifflabs.dev instead of a
personal mailbox: the packaging metadata, the `__author__` string, and the
README's contact section.
Also removes the last two pieces of personal data in the tree:
- clusters.toml pinned a site-specific login name on the barrine profile. That
file is shared, so a login belongs in a personal override; the header now
documents ~/.config/cme/clusters.toml for exactly this. {user} falls back to
the running user's login, which is the sensible default anyway.
- a commented-out line in the Gadget tab still carried an absolute path under
one user's home directory.
AUDIT.md's description of finding 3.4 no longer enumerates the actual logins,
hostnames and home directories -- it makes the same point by counting them.
Test changes that follow:
- the profile-level `username` feature is still covered, but against an inline
config rather than a shipped profile that should not pin one.
- a new test asserts the shipped profiles resolve {user} to whoever is running.
- the "no personal home directory" test previously grepped resolved paths for a
specific surname. It now resolves each profile as two different users and
asserts the results differ only where the username appears, which is the
actual property wanted and does not need the name to state it.
Left in place: the github.com/bgriffen/cme URLs, which are the repository's real
location, and the LICENSE copyright holder, which needs to name a person.
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.
Audits the codebase, fixes the defects it turned up, and pulls the science and
file-format generation out of the GUI event handlers so they can be tested.
18 commits, each self-contained. Worth reading in order — the refactors come
before the fixes they make verifiable.
Why
CME could not run. The
modulesI/O package it depends on is not in therepository, four of eleven files did not parse on Python 3, and the
enthought.*Traits namespace it imports was retired in 2011.The deeper problem was structural: every piece of science and every
configuration-file writer was welded to a button handler, interleaved with
figure.clear()and axis labelling. Nothing could be tested, scripted orreused. That is what let a mis-indented block leave the Plot button doing
nothing in parent mode, and a one-character typo leave the entire Gadget
parameter sweep inert, for a decade.
What changed
Runnable on a current interpreter. 35 print statements, 6
xrange, 21pandas
.ix→.loc.enthought.traits.api→traits.apiand friends.wxPython is kept as the toolkit — a Qt port is a large diff that cannot be
verified without a display, so it is deliberately deferred.
Science and writers extracted, and tested. Everything outside
cme/ui/isnow display-free: plain functions over arrays and scalars, no Traits, no
matplotlib.
cme.namingcme.cosmologygrifflib.cosmoconstant(absent) + 6 hardcoded0.6711cme.writersf.writecalls forparam.txt, 90 forConfig.sh, 50 per MUSIC confcme.science.candidatescme.shellos.system/shell=Truecalls on concatenated pathscme.configplatform.node()with 19 personal identifiersclusters.toml171 tests, from zero. Golden-file comparison for a MUSIC conf; the Gadget
parameter file and
Config.share parsed back rather than string-matched.Several tests are pinned regressions and cite their
AUDIT.mdsection.Defects fixed
All 18 from the audit, plus 12 found while doing the work. The ones that
mattered most:
self.nvir = [...]where the traitis
nrvir. Traits accepts assignment to an undeclared attribute by creatingit, so
nrvirstayed an empty list and every sweep loop body was skipped.The tab's main action was inert.
'#SBATCH -n ' + self.SLURMcoresraises
TypeErroron an integer trait. Itsmpirunline was also built fromthe PBS core count, and enabling SUBFIND left it pointing at the wrong
binary because both branches of
_SLURMcores_changedwere identical.candidatearrwas seededwith
np.zeros((1,6))and grown byvstack, so a zero-mass halo at theorigin was plotted, counted, and addable to the working sample as ID 0.
z = tmphalos['posY']plotted y twice."groups_0" + str(n)is only correctfor two-digit numbers: snapshot 5 looked for
groups_05where Gadget writesgroups_005, and the zoom tabs' default of 255 producedsnapdir_0255._Z127was hardcoded in the contamination and halo tabs rather than readfrom their own
zinit, so runs at another starting redshift were looked up ina directory that had never been created.
HubbleParamheld H0 in a field written as h, so the value shown in theGUI disagreed with the value on disk by a factor of 100.
for lmini in self.lminiterated a string, so a levelmin of 10 or aboveran the sweep twice with
'1'and then'0'.Plus:
UnboundLocalErroringethostid,NameErroron every parent full-boxplot, three MUSIC Poisson settings silently discarded,
Clearwriting to astray attribute on the wrong object, and
ApplicationMain.__init__nevercalling
HasTraits.__init__.Security and robustness
70 call sites assembled shell commands by concatenating paths from Directory
traits. A path with a space split the command; a path with
;or$(...)executed. All now run as argument lists with an explicit
cwd. Theos.chdir/chdir-back pairs ininstall.pywere not exception-safe, and the;-joined command strings ranmake installeven afterconfigurehad failed.19 personal identifiers — usernames, hostnames, absolute home directories and
two email addresses — moved into
clusters.toml.Performance
The Lagrangian region builder re-read four large blocks from the parent
simulation on every iteration of the
nrvirloop, none of which depend onnrvir. For a 512³ parent that is ~30 GB of I/O for a five-value sweep tocompute the same arrays five times. Hoisted, with no increase in peak memory.
Candidate selection went from O(N²) to O(N).
What is deliberately not done
modulesI/O package is still absent. The GUI still cannot start froma clean checkout. This needs vendoring or pinning and is the top item in
AUDIT.md.verified by
compilealland inspection only, because exercising them needs adisplay and a real parent simulation. Changes under
cme/ui/carry real riskand want a smoke test on a machine that has the readers.
package manager.
Full list, with the reasons for each deferral, in
AUDIT.md.Two calls worth confirming
LICENSE, copyright 2014-2026 Brendan Griffen.Swap for BSD-3-Clause if you prefer.
grifflibis absent, these weretaken from the cited papers rather than copied from the original. PLANCK
cross-checks exactly against the values that were hardcoded in the Gadget tab
(Ω_m = 0.3175, Ω_Λ = 0.6825, h = 0.6711), and its Ω_b agrees with Planck
2013's Ω_b h² = 0.02205. The WMAP rows have no such cross-check — diff them
against
grifflibbefore trusting them for new science. Flagged in themodule docstring.
Verification
CI runs all of the above on Python 3.9, 3.11 and 3.12.